Element groups#139
Conversation
jmaack24
left a comment
jmaack24
left a comment
There was a problem hiding this comment.
I like this feature. I think forcing a group to be an interval of element ids is too rigid though. It would make grouping difficult to use in a python script and nearly impossible to use with the GUI.
There is also no implementation done for the NativeRunner or EmbreeRunner. At a minimum, there should be some sort of error printed or thrown for those two runners that say this feature has not been implemented. From the user survey results, I expect these are going to be the most commonly used runners by users.
| return static_cast<double>(sqrtf(cross.x * cross.x + cross.y * cross.y + cross.z * cross.z)); | ||
| } | ||
|
|
||
| int8_t SolTraceSystem::get_group(int32_t element_id) { |
There was a problem hiding this comment.
This should live in OptixRunner rather than here. See comment on line 109 of soltrace_system.h
Again, I think we need a grouping implementation that is more flexible.
|
|
||
| # Conditionally link OptiX runner | ||
| if(SOLTRACE_BUILD_OPTIX_SUPPORT) | ||
| list(APPEND RES_LIBS optix_runner) |
There was a problem hiding this comment.
SimulationResult unit tests shouldn't need a runner.
They should be pretty simple tests which check that a particular function works. The idea is to make sure the function does what it "promises" to do while also making you think about whether you've handled the possible error conditions. See the other SimulationResult, SimulationData or even many of the NativeRunner unit tests. Do NOT use the CST unit tests as an example (most of those are examples of what NOT to do).
If you absolutely have to use a runner, it should be the NativeRunner so these tests can run on any platform.
| params.max_number_of_rays = params.number_of_rays * 100; | ||
| params.seed = 608; | ||
|
|
||
| OptixRunner runner; |
There was a problem hiding this comment.
This will fail to compile if we are building without optix support.
There was a problem hiding this comment.
This test feels much more like a test that should be in the OptixRunner unit test group.
|
Should we change the JSON schema with this pull request? That process is not defined, but it seems like adding json variables would be a good time to increment it. @jmaack24 |
We probably should come up with a process for changing the JSON schema or some sort of versioning for it. What were you thinking of adding? |
I don't have a plan in mind. I'll add a note so we remember to discuss it at the next meeting. |
…ion call name in test
3 participants
.gitignoredidn't ignore it--leveland--verbosetosimdriver\main.cpp. formain,--levelchanges the file printing behavior. i haven't actually implemented--verboseyetgroupto elements to programmatically track groups of elements. i tried to make this backwards compatible, so simulations without groups wouldn't need to be changed. currently groups must be added through the JSON file, after all ungrouped elements, in order from 0 to the number of groups (seefile_io_test.cppfor the tests i wrote regarding this).simulation_data.cpp. how should i get the system to fail gracefully from the users perspective?single_element.cpp, composite/stage/etc elements are given -2 because they don't interact with the tracestruct SolTrace::Result::GroupResultto hold the counts for a given group. right now it just has basic counts (ie absorbs/reflects), but i will write a function for comparing GroupResult structs to get stuff like efficiencies.GroupResultstoSimulationResultand createdSimulationResult::write_json_fileto write those structs to a json fileOptixCSP::SolTraceSystem, for the other runners, add the groups toTSystemfor what i think would be a similar level of access.enum SolTrace::Runner::RunnerStatisticsto toggle between different reporting behavior, ie doesrunner.report_simulation(&result, level)return the ray record data, the group results, or both?\unit-tests, added some json files for testing file io behavior