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

Handle crashing micro simulations #85

Merged
merged 23 commits into from
May 3, 2024
Merged

Handle crashing micro simulations #85

merged 23 commits into from
May 3, 2024

Conversation

tjwsch
Copy link
Collaborator

@tjwsch tjwsch commented Mar 19, 2024

This is an attempt to solve issue #74.
This cannot be a permanent solution to the handling of crashing simulation. If a simulation crashes during the first time step, it is replaced with data from another random simulation. In the long run, I don't see any other solution than an interpolation (in cases where only one simulation is given and it cannot be replaced by one of different complexity).
Moreover, it has not been tested with real simulations, in parallel, or using spack.

@IshaanDesai IshaanDesai added the new-feature Adding a new feature label Mar 28, 2024
@IshaanDesai IshaanDesai marked this pull request as ready for review March 28, 2024 09:57
@IshaanDesai IshaanDesai self-requested a review March 28, 2024 09:58
Copy link
Member

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

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

Good first steps, but we need to do a bit more work before merging this. Please look at the comments below. A general comment is writing comments in the code such that it is readable. I will do another round of reviewing soon.

@tjwsch
Copy link
Collaborator Author

tjwsch commented Apr 9, 2024

The latest commit adds interpolation. Besides lacking extrapolation, scipy's griddata module also cannot interpolate on linearly dependent data, thus data on the dummy problems cannot be interpolated. more thought must be put into the issue, to interpolate and extrapolate all cases of simulation crashes, and implementing a custom solution for interpolation is most likely required.

@IshaanDesai
Copy link
Member

Further discussions led to the idea that implementing an inverse distance weighing would make sense for the interpolation. This would be done with a k-nearest-neighbor method, where the user can set k.

@tjwsch tjwsch force-pushed the handle_crashing_sim branch 2 times, most recently from 40c1f95 to e6c3baf Compare April 11, 2024 14:42
@tjwsch tjwsch requested a review from IshaanDesai April 16, 2024 13:02
micro_manager/micro_manager.py Outdated Show resolved Hide resolved
micro_manager/micro_manager.py Outdated Show resolved Hide resolved
Copy link
Member

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

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

Good effort already 👍 my suggestions are mainly about code styling. Let us test this functionality on a real example and check if crashes are caught.

Copy link
Member

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

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

We are getting closer to merging this 😁 some more comments on my side. The logic and overall code looks good now.

@tjwsch tjwsch requested a review from IshaanDesai May 3, 2024 08:32
Copy link
Member

@IshaanDesai IshaanDesai left a comment

Choose a reason for hiding this comment

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

Good to go 👍 please remember to squash and merge.

@tjwsch tjwsch merged commit 407809c into develop May 3, 2024
10 checks passed
@tjwsch tjwsch deleted the handle_crashing_sim branch May 3, 2024 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature Adding a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants