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

Avoid crashes in plotting, do not plot residuals of mask or bad times #127

Merged
merged 3 commits into from
Sep 8, 2022

Conversation

jzuhone
Copy link
Collaborator

@jzuhone jzuhone commented Sep 7, 2022

Description

For models which execute very quickly, it is sometimes the case that during fitting a race condition of sorts is encountered when a plot that is being updated and a reference to the canvas object associated with the plot is lost (see Issue #126).

Unfortunately, the cause for this is still unknown. I was able to fix this problem by 1) Removing unnecessary references to the canvas object I was keeping around and also calling canvas.flush_events after every update.

This PR also adds some logic that affects the resid__data and histogram/dashboard plots in xija_gui_fit by masking out data that is under a "bad time" or a "mask time". The residuals are set to zero in such cases, but because of that they still appear in the plot and can give default plot limits that are unhelpful. We copy the residual data before plotting and set the masked data to NaN here.

Fixes #126

Testing

Functional tests

Fitted the 1DPAMZT model over several iterations, fit the WIP HRC model which was crashing previously, crashes no longer occur.

@jzuhone
Copy link
Collaborator Author

jzuhone commented Sep 7, 2022

It would probably be better to handle this with some kind of timer, but the examples I looked at online were rather complex and I wasn't sure they would work here. Open to suggestions.

@taldcroft
Copy link
Member

I rebased #125 on this branch and was able to successfully fit the HRC CEA model for both 100 days and 16 days with no crash.

I agree this fix feels a bit ad hoc and possibly prone to failures... but I don't have any better ideas and it does seem to work. So I'm approving but it would be good to get confirmation from Dan.

@taldcroft taldcroft self-requested a review September 7, 2022 19:10
@taldcroft
Copy link
Member

taldcroft commented Sep 7, 2022

Scratch that. Using this branch rebased on #125 I got the error for the HRC model (100 days) by deleting the DPA plot and then trying to add the 2CEAHVPT resid vs time plot.

@jzuhone jzuhone changed the title Slow down model plotting to avoid crashes, do not plot residuals of mask or bad times Avoid crashes in plotting, do not plot residuals of mask or bad times Sep 7, 2022
@jzuhone
Copy link
Collaborator Author

jzuhone commented Sep 7, 2022

@taldcroft I think I found the real fix. You'll have to delete the original version of this branch to try it because I rewrote the history. Look at the diffs and you can see that this is a much better solution.

@taldcroft
Copy link
Member

I won't say that I fully understand why that works (why that ref was causing crashes), but it certainly looks a lot better. Nice!

I have been using it for 5 minutes now with no crashes.

@jzuhone
Copy link
Collaborator Author

jzuhone commented Sep 7, 2022

I think it's flush_events that's doing the heavy lifting here, but I also thought it was unnecessary and possibly error-prone to keep the extra canvas attributes around.

@drxre
Copy link

drxre commented Sep 7, 2022

I was able to install and run it successfully. Thank you for all the hard work!

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.

RuntimeError trying to run xija_gui_fit on Mac
3 participants