Feature/haluska2/sina curve ordering #1559
Conversation
src/axom/sina/core/Record.cpp
Outdated
| // Note: the toNode is being used as an intermediary here. As such, we force the CurveOrder to be oldest | ||
| // first, aka "don't reorder". If the user wants the curves in a specific order, that's decided at dump time, | ||
| // else the Record will have no way to know the "original" order within the library. | ||
| auto newLibData = addLibraryData(name, childRecord.toNode(CurveSet::CurveOrder::REGISTRATION_OLDEST_FIRST)); |
There was a problem hiding this comment.
Reviewer context: library data represents a sort of "sub-record", specifically info a library might be responsible for that should be available with the host code's.
|
@HaluskaR this fails a lot of tests. |
|
/style |
|
Anyone else I should request a review from? If not, I'm going to make a push for the curveNames/orderedCurveNames changes |
Can @doutriaux1 and @bgunnar5 please give it a first review? |
There was a problem hiding this comment.
@HaluskaR this orders curves within a curveset, do we also want to order curvsets?
otherwise it looks good, you might (or not) want to add the sorting functions I mentioned.
Also can we add a case where the user passes the order in which the curves need to be stored? that would allow them the most flecibility w/o having you implement new requests in the future
Great question on the curve set ordering, I was thinking about that...we haven't seen tons of use of separate curve sets thus far so it's hard for me to make the call on what users would expect. My temptation would be to wait to see if there's a need, since I could see the idea of "order by age" being less obvious than for its curves (creation time vs. when an/the independent added vs. when a curve was last added into it). Custom curve ordering's a good plan, adding that in now. Implementation thoughts from me: If we have dependents: "x_vel", "y_vel", "z_vel" and the user gives us dependents: "y_vel", "z_vel" or dependents: "z_vel", "y_vel", "x_vel", "q_vel" Reject either, lists need to match exactly, return a bool. Call it applyCustom[In]DependentOrder. It'll take me a sec to add it so let me know if you want something other than the above! |
@doutriaux1 Tagging for the above comment |
|
/style |
|
@kennyweiss We're good on reviews from our side. |
|
@HaluskaR don't we need the append PR in first? To double check this will work properly then? |
I believe the concern was that the append code was assuming some order in the record it was appending into, but that didn't look to be the case. If it's more that the appended order could introduce new curves that don't fit the dumped order (i.e. we dumped alphabetically to a record ending with the curve zebra_spottings, then appended in the curve apple_juice_consumption, and that would go in AFTER), I do believe that would be the case but would require a change on this PR, as right now any record node Sina's handed is assumed to be stored in oldest_first, so we'd need to shove in some metadata of our own. This might be a bit of a YMMV if someone's planning to be constantly appending in new curves. In that case, if they want anything other than oldest first, I might consider that an issue for post processing; how can we know when a file's "done" from Sina's end? Could definitely be overlooking a concern though, let me know! |
|
You maybe right. I sort of remembered something about asking for order that would be different from what's in the file in append mode, but I think you're right for json we will rewrite the whole file anyway so we can change the order and for hdf5 I don't think the order matters. Does the HDF5 dump does anything in particular for the ordering? If not we're probably good, but I'd like to be sure firs.t |
Thanks -- in that case, could @doutriaux1 and/or @bgunnar5 please approve the PR? |
There was a problem hiding this comment.
Thanks @HaluskaR !
Please update the RELEASE_NOTES, and mention any API changes that users should be aware of.
| { | ||
| auto &curveName = curve.getName(); | ||
| auto existing = curves.find(curveName); | ||
| if(existing == curves.end()) | ||
| { | ||
| curves.insert(std::make_pair(curveName, curve)); | ||
| nameList.emplace_back(curveName); |
There was a problem hiding this comment.
Should this be push_back since presumably, curveName still belongs to curve ?
Summary
Could use ideas for additional useful orderings if any, getting things in the order the ULTRA users expect is the main goal.