8000 add align argument to compare() · Issue #214 · larray-project/larray-editor · GitHub
[go: up one dir, main page]
Skip to content

add align argument to compare() #214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
gdementen opened this issue Mar 12, 2021 · 4 comments
Open

add align argument to compare() #214

gdementen opened this issue Mar 12, 2021 · 4 comments
Labels
difficulty: low enhancement priority: high work in progress
Milestone
0.35

Comments

@gdementen
Copy link
Collaborator
gdementen commented Mar 12, 2021 •
edited
Loading

Our users often compare variants but also old runs for their models which do not have the same axes (especially time range) than the current model results.

See larray-project/larray#501
Note that larray-project/larray#463 would bring that for free.
@gdementen gdementen changed the title compare sessions align all arrays compare sessions should align all arrays Mar 12, 2021
@gdementen gdementen added this to the 0.35 milestone Sep 16, 2022
@gdementen gdementen changed the title compare sessions should align all arrays compare should align arrays Nov 28, 2022
@gdementen gdementen changed the title compare should align arrays support aligning arrays in compare Nov 28, 2022
@gdementen
Copy link
Collaborator Author
  • This should work for both comparing sessions or arrays.
  • This should be optional (with either an argument align=XYZ, or check_axes=XYZ to check the axes are the same (which is essentially the same thing). I am totally unsure which should be the default though:
    • some users use compare to check whether the axes are the same and if so check the differences, so would be annoyed by a change in behavior.
    • some users align manually before comparing (or worse: do not use compare because it is too cumbersome to do that), so would benefit from align by default.

@alixdamman: what do you think?

@alixdamman
Copy link
Contributor

I find aligning arrays and sessions in compare() is really good idea.

This should work for both comparing sessions or arrays.

Indeed.

This should be optional (with either an argument align=XYZ, or check_axes=XYZ to check the axes are the same (which is essentially the same thing). I am totally unsure which should be the default though:
- some users use compare to check whether the axes are the same and if so check the differences, so would be annoyed by a change in behavior.
- some users align manually before comparing (or worse: do not use compare because it is too cumbersome to do that), so would benefit from align by default.

I would first add an argument align to the compare() function with possible values None, 'inner', 'outer', 'left' and 'right' + a ComboBox with the same choices in the compare() window.

The remaining question is what to choose as default value for the argument align (and so the ComboBox).
IMO, it should be 'outer'. In case of different labels, the corresponding values (nan) will appear as different indicationg there is a possible mismatch between labels. If the users are enough smart, they would be able to understand the problem because of the nan.
It is impossible to satisfy all users. To me, the more logical is to take 'outer' as default value as it is the most informative check the difference in values and labels).

Feel free to have your own opinion. You have mine.

@gdementen
Copy link
Collaborator Author

I would first add an argument align to the compare() function with possible values None, 'inner', 'outer', 'left' and 'right' + a ComboBox with the same choices in the compare() window.

I thought about making the argument a boolean but your idea is much better. And making a combo box for it is a good idea too... but I am unsure where to place the combobox, especially for the Session comparator. Will have to think about this. I think we could first implement the argument, then the combobox.

@alixdamman
Copy link
Contributor

I think we could first implement the argument, then the combobox.

I agree.

@gdementen gdementen mentioned this issue Dec 6, 2022
Open
@gdementen gdementen removed the question label Dec 6, 2022
@gdementen gdementen changed the title support aligning arrays in compare add align argument to compare() Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
difficulty: low enhancement priority: high work in progress
Projects
None yet
Milestone
0.35
Development

No branches or pull requests
2 participants
@gdementen @alixdamman
0