Skip to content
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.

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

Add residual plot overloading the - operator #275

Closed
wants to merge 12 commits into from
Closed

Conversation

nvaytet
Copy link
Member

@nvaytet nvaytet commented Nov 3, 2023

Note that this was just to try things out.
Decision on whether we should implement it this way has not yet been made.

Fixes #201

@nvaytet nvaytet requested a review from jl-wynen November 16, 2023 12:15
@nvaytet nvaytet marked this pull request as ready for review November 16, 2023 13:46
@jl-wynen
Copy link
Member

jl-wynen commented Jan 8, 2024

Sorry for the late reply. This got lost in my feed somehow.

I don't think using the - operator like this is a good idea. The other operators only compose plots. But here, it adds a new line to an existing plot and computes new data for the residuals plot.

This solution optimises completely for modularity but, as you said, it's expensive because of the temporary plots. Would it be acceptable to modify the input plot by adding a line to it? This would give more or less the same performance as a dedicated function as first described in #201 but would also be modular. (In this case, - would definitely be a bad choice because it doesn't look like it modifies the input.)

@nvaytet
Copy link
Member Author

nvaytet commented Jan 9, 2024

Sorry for the late reply. This got lost in my feed somehow.

I don't think using the - operator like this is a good idea. The other operators only compose plots. But here, it adds a new line to an existing plot and computes new data for the residuals plot.

This isn't technically modifying the original plot, it makes a new plot and copies the data over from the old plots, which should still be displayable in their original form.

This solution optimises completely for modularity but, as you said, it's expensive because of the temporary plots. Would it be acceptable to modify the input plot by adding a line to it? This would give more or less the same performance as a dedicated function as first described in #201 but would also be modular. (In this case, - would definitely be a bad choice because it doesn't look like it modifies the input.)

Maybe this isn't so expensive anymore after #290 ?

@jl-wynen
Copy link
Member

jl-wynen commented Jan 9, 2024

This isn't technically modifying the original plot, it makes a new plot and copies the data over from the old plots, which should still be displayable in their original form.

True. But it does compute new data which the other operators don't do.

Maybe this isn't so expensive anymore after #290 ?

Do you have benchmarks?

@nvaytet
Copy link
Member Author

nvaytet commented Jan 16, 2024

Do you have benchmarks?

Nothing beyond what I mention in the PR message:

  • Creating the 20 subplots now takes a little over 1s as opposed to 1 minute before.
  • As a bonus, running the unit tests now takes 30s instead of a minute

main_fig:
The main figure.
reference:
The reference figure.
Copy link
Member

Choose a reason for hiding this comment

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

Missed this earlier, but wouldn't it make more sense to accept a data array instead of a figure? Why would you require the user to plot the reference data first?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm starting to think that the whole approach using operators is a bad idea. I also ran into problems if, for example, the user made a figure where they changed the line style or the line color. Then I had to make sure that was copied over.

I think we need to go back to some of the suggestions in #201 and use a dedicated function, and drop the operators.

ref_node = next(iter(reference.graph_nodes.values()))
data = ref_node()
if not data.name:
data.name = "reference"
Copy link
Member

Choose a reason for hiding this comment

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

This modifies the input, right? Seems like you should make a copy of reference first.

@nvaytet nvaytet closed this Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a new residuals_plot?
2 participants