Skip to content

1824 add solution comparison tool#3083

Merged
jonmaddock merged 13 commits intomainfrom
1824-add-solution-comparison-tool
May 1, 2024
Merged

1824 add solution comparison tool#3083
jonmaddock merged 13 commits intomainfrom
1824-add-solution-comparison-tool

Conversation

@jonmaddock
Copy link
Copy Markdown
Contributor

@jonmaddock jonmaddock commented Feb 29, 2024 •
edited
Loading

Add the ability to plot multiple solution vectors, normalised or unnormalised, with objective functions and RMS errors.

@jonmaddock jonmaddock self-assigned this Feb 29, 2024
@jonmaddock jonmaddock linked an issue Feb 29, 2024 that may be closed by this pull request
Add solution comparison tool #1824
Closed
@jonmaddock jonmaddock force-pushed the 1824-add-solution-comparison-tool branch 15 times, most recently from d6aab99 to 2643607 Compare March 6, 2024 12:13
@jonmaddock
Copy link
Copy Markdown
Contributor Author

jonmaddock commented Mar 6, 2024

It would be better to test this notebook, but also notebooks generally in Process, using the tools in #3091.

@jonmaddock jonmaddock force-pushed the 1824-add-solution-comparison-tool branch 2 times, most recently from c8ff616 to 343e009 Compare March 6, 2024 15:15
@jonmaddock jonmaddock marked this pull request as ready for review March 11, 2024 10:37
@jonmaddock jonmaddock requested a review from timothy-nunn March 11, 2024 10:40
@jonmaddock
Copy link
Copy Markdown
Contributor Author

jonmaddock commented Mar 11, 2024

@timothy-nunn you reviewed a previous version of this for me here, but I've made quite a few changes that I think improve on that and go beyond it. I'd be grateful for your review, thanks.

timothy-nunn
timothy-nunn requested changes Mar 15, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@timothy-nunn timothy-nunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jon, looks good other than a couple typing mistakes, possible (very unlikely) bugs, and a bug with the figure annotation.

The only thing I wonder about is if we really want to have an implicit normalising_tag if normalise=True but its not provided? I lean towards explicit in cases like this and the behaviour would be easier to understand if we only normalise if a normalising_tag is provided. What are your thoughts on this?

Comment thread tests/integration/test_examples.py
Comment thread tests/integration/test_plot_solutions.py Outdated
Comment thread process/io/plot_solutions.py Outdated
Comment thread process/io/plot_solutions.py Outdated
Comment thread process/io/plot_solutions.py
Comment thread process/io/plot_solutions.py Outdated
Comment thread process/io/plot_solutions.py Outdated
Comment thread process/io/plot_solutions.py
Comment thread process/io/plot_solutions.py Outdated
Comment thread process/io/plot_solutions.py
@jonmaddock
Copy link
Copy Markdown
Contributor Author

jonmaddock commented Apr 26, 2024

Hi Jon, looks good other than a couple typing mistakes, possible (very unlikely) bugs, and a bug with the figure annotation.

The only thing I wonder about is if we really want to have an implicit normalising_tag if normalise=True but its not provided? I lean towards explicit in cases like this and the behaviour would be easier to understand if we only normalise if a normalising_tag is provided. What are your thoughts on this?

I agree; it would be much better to simplify and just have a normalising_tag argument: I'll change this.

@jonmaddock jonmaddock force-pushed the 1824-add-solution-comparison-tool branch from b87bafb to 1041119 Compare April 26, 2024 09:59
@jonmaddock
Copy link
Copy Markdown
Contributor Author

jonmaddock commented Apr 26, 2024

Hi Tim, thanks for you patience with this; this is ready for review again.

@jonmaddock jonmaddock requested review from timothy-nunn and removed request for timothy-nunn April 26, 2024 10:00
@jonmaddock
Copy link
Copy Markdown
Contributor Author

jonmaddock commented Apr 26, 2024

Apologies, fixing tests.

@jonmaddock jonmaddock force-pushed the 1824-add-solution-comparison-tool branch from 1041119 to b65ad4b Compare April 26, 2024 10:19
@jonmaddock
Copy link
Copy Markdown
Contributor Author

jonmaddock commented Apr 26, 2024

Ready for review again Tim. Regression tests are failing due to adding objf_name to the MFILE output, which isn't in the reference MFILE.

@jonmaddock jonmaddock requested a review from timothy-nunn April 26, 2024 11:34
timothy-nunn
timothy-nunn approved these changes May 1, 2024
View reviewed changes
Copy link
Copy Markdown
Collaborator

@timothy-nunn timothy-nunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Happy with these changes, especially the simplification of turning on/off normalisation. Cheers Jon.

@jonmaddock jonmaddock merged commit 8697602 into main May 1, 2024
@jonmaddock jonmaddock deleted the 1824-add-solution-comparison-tool branch May 1, 2024 15:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timothy-nunn timothy-nunn timothy-nunn approved these changes

Assignees

@jonmaddock jonmaddock

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add solution comparison tool

2 participants

@jonmaddock @timothy-nunn