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

Always export final solution #118

Merged
merged 4 commits into from
Mar 5, 2024
Merged

Always export final solution #118

merged 4 commits into from
Mar 5, 2024

Conversation

ddundo
Copy link
Member

@ddundo ddundo commented Mar 3, 2024

Closes #106

With this PR the final solution of each subinterval always gets exported, instead of the first one.

Also tagging @stephankramer and @acse-ej321 for awareness :)

@ddundo ddundo requested a review from jwallwork23 March 3, 2024 22:48
@ddundo ddundo linked an issue Mar 3, 2024 that may be closed by this pull request
@ddundo ddundo changed the title Always export final solution (#106) Always export final solution Mar 4, 2024
Copy link
Member

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

Thanks for this @ddundo.

Please could you use reversed instead of np.flip? Good to use Python build-ins rather than external packages, where possible.

Could you also do the same thing in adjoint.py?

@ddundo
Copy link
Member Author

ddundo commented Mar 4, 2024

Ah sorry @jwallwork23, tunnel vision here! I was looking into adjoint.py now and I'm not sure I understand Line 369...

Shouldn't it be project(sols.adjoint[i + 1][0], sols.adjoint_next[i][j]) instead of project(sols.adjoint_next[i + 1][0], sols.adjoint_next[i][j])? That is, the adjoint_next on the final subinterval timestep is the adjoint on the first timestep in the next subinterval?

@ddundo ddundo requested a review from jwallwork23 March 5, 2024 04:34
@ddundo
Copy link
Member Author

ddundo commented Mar 5, 2024

This should be ready for a re-review now @jwallwork23 :)

I also made the change I mention in the above comment. It looked like a bug to me

Copy link
Member

@jwallwork23 jwallwork23 left a comment

Choose a reason for hiding this comment

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

Thanks very much for this @ddundo! Your bugfix and test tweaks make sense to me.

Just two very minor requests. If you fix those then feel free to merge.

Comment on lines 573 to 575
for j, block in zip(
range(num_exports - 1), reversed(solve_blocks[::-stride])
):
Copy link
Member

Choose a reason for hiding this comment

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

One final thing for conciseness: could we switch this to

for j, block in enumerate(reversed(solve_blocks[::-stride])):

so that it (hopefully) fits on one line? My attention was brought to this because I was concerned that zip doesn't care if the iterables have different lengths so might be skipping some iterations, but the check above ensures things are okay.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice! Done in eb187c0

Comment on lines 339 to 341
for j, block in zip(
range(num_exports - 1), reversed(solve_blocks[::-stride])
):
Copy link
Member

Choose a reason for hiding this comment

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

Similarly here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in eb187c0

@jwallwork23 jwallwork23 added the bug Something isn't working label Mar 5, 2024
@jwallwork23 jwallwork23 added this to the Version 1 milestone Mar 5, 2024
@ddundo
Copy link
Member Author

ddundo commented Mar 5, 2024

Thanks Joe! I did as you suggested and will merge :)

@ddundo ddundo merged commit 670a4e5 into main Mar 5, 2024
1 check passed
@ddundo ddundo deleted the 106-saving-final-solution branch March 5, 2024 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Saving final solution
2 participants