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.

Sign up for GitHub

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

Jump to bottom

Update TimeStepWizard #3248

Merged
merged 16 commits into from
Sep 6, 2023
Merged

Update TimeStepWizard #3248

merged 16 commits into from
Sep 6, 2023

Conversation

iuryt
Copy link
Collaborator

@iuryt iuryt commented Aug 31, 2023 •
edited
Loading

Reopens #3244 and #3245

If I understand it correctly, it does not make sense to have min_change > 1 or max_change < 1. Am I right?
If so, should we add at least a warning for those cases?

Please, let me know if I 'reopened' the PR correctly.

@glwagner
Copy link
Member

If I understand it correctly, it does not make sense to have min_change > 1 or max_change < 1. Am I right?
If so, should we add at least a warning for those cases?

Right, it doesn't make sense. But I think we should throw(ArgumentError rather than merely warn because that can do bad things like spuriously increase your time-step.

@glwagner
Copy link
Member

Some stuff later in the docstring is also weird. For example it says

To use TimeStepWizard, adapt in a Callback and add it to a Simulation:

I don't know what it means to "adapt" a function in a callback. There's no adaptation, TimeStepWizard was born for Callback!! Better to say that we "wrap" or "insert it" into Callback?

@iuryt
Copy link
Collaborator Author

iuryt commented Sep 1, 2023

Some stuff later in the docstring is also weird. For example it says

To use TimeStepWizard, adapt in a Callback and add it to a Simulation:

I don't know what it means to "adapt" a function in a callback. There's no adaptation, TimeStepWizard was born for Callback!! Better to say that we "wrap" or "insert it" into Callback?

Good catch! I will work on the docstring and error after the weekend.

@iuryt iuryt changed the title Update docstring of the TimeStepWizard Update docstring of the TimeStepWizard and raise error for invalid min_change or max_change values Sep 2, 2023
@iuryt iuryt changed the title Update docstring of the TimeStepWizard and raise error for invalid min_change or max_change values Update TimeStepWizard Sep 2, 2023
@iuryt
Copy link
Collaborator Author

iuryt commented Sep 2, 2023

I believe I implemented everything we discussed. Please, let me know if that's seems fine to be merged.

iuryt and others added 2 commits September 3, 2023 08:24
@iuryt @navidcy
Fix grammar for the docstring
de9c96c 
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
@iuryt @navidcy
Make max_change and min_change error simpler
22e2e06 
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
glwagner
glwagner approved these changes Sep 4, 2023
View reviewed changes
Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

Great improvement in docs and input validation

@iuryt
Copy link
Collaborator Author

iuryt commented Sep 4, 2023

One of the errors is for the Stratified Couette flow validation.
But max_change = 1.1 in
https://github.com/iuryt/Oceananigans.jl/blob/22e2e06b4f1451c18cfd4489a8862e28faab0759/validation/stratified_couette_flow/stratified_couette_flow.jl#L245

I suggest to add in the error output which value was given by the user.

@iuryt @simone-silvestri
Fix to the correct sign for the error message
86fd5d1 
Co-authored-by: Simone Silvestri <silvestri.simone0@gmail.com>
@iuryt
Copy link
Collaborator Author

iuryt commented Sep 4, 2023

How does that work for Julia?
For Python, if the user gives both max_change<1 and min_change>1, this is going to give the max_change error only. Is it the same for Julia? If so, should we keep like this?

@navidcy
Copy link
Collaborator

navidcy commented Sep 4, 2023 •
edited
Loading

How does that work for Julia?
For Python, if the user gives both max_change<1 and min_change>1, this is going to give the max_change error only. Is it the same for Julia? If so, should we keep like this?

Sorry that was my bad for the error.

This

a==b && foo

means that foo will be executed only if a==b. See:

https://docs.julialang.org/en/v1/manual/control-flow/#Short-Circuit-Evaluation

But to answer your question: yes lets keep it like that. User will get another error next time :)

@iuryt
Copy link
Collaborator Author

iuryt commented Sep 4, 2023

Ok. I was thinking that if, for some reason, the user giver both max_change < 1 and min_change > 1, this is going to give a partial error. But well, the user can just try with a different max_change and then get the error for min_change and solve for both.

@iuryt @glwagner
Update docstring
a0d53ca 
Co-authored-by: Gregory L. Wagner <gregory.leclaire.wagner@gmail.com>
@glwagner
Copy link
Member

glwagner commented Sep 5, 2023

Ok. I was thinking that if, for some reason, the user giver both max_change < 1 and min_change > 1, this is going to give a partial error. But well, the user can just try with a different max_change and then get the error for min_change and solve for both.

Note this is common. If your code has many bugs, you'll typically only be able to receive errors from one at a time.

@navidcy
Copy link
Collaborator

navidcy commented Sep 5, 2023

@iuryt do you have write rights?

@iuryt
Copy link
Collaborator Author

iuryt commented Sep 5, 2023

@iuryt do you have write rights?

nope

iuryt and others added 2 commits September 5, 2023 17:08
@iuryt
Merge branch 'main' into main
5da3823
@iuryt @navidcy
Remove extra space
b4fd9f8 
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
@glwagner glwagner merged commit 4a31c0c into CliMA:main Sep 6, 2023
siddharthabishnu pushed a commit that referenced this pull request Sep 11, 2023
@iuryt @navidcy
@simone-silvestri @glwagner @siddharthabishnu
Update TimeStepWizard (#3248)
1faa184 
* Update docstring of the TimeStepWizard

* Update TimeStepWizard explanation in the horizontal_convection example

* Change symbol for 'less than or equal' and 'greater than or equal'

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>

* Change symbol for 'less than or equal' and 'greater than or equal'

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>

* better phrasing

Co-authored-by: iury simoes-sousa <iuryt@pm.me>

* Update docstring of the TimeStepWizard

* Raise ArgumentError for invalid max_change or min_change values

* Fix wording in the docstring

* Fix grammar for the docstring

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>

* Make max_change and min_change error simpler

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>

* Update output error for TimeStepWizard

* Fix to the correct sign for the error message

Co-authored-by: Simone Silvestri <silvestri.simone0@gmail.com>

* Update docstring

Co-authored-by: Gregory L. Wagner <gregory.leclaire.wagner@gmail.com>

* Remove extra space

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>

---------

Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Simone Silvestri <silvestri.simone0@gmail.com>
Co-authored-by: Gregory L. Wagner <gregory.leclaire.wagner@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simone-silvestri simone-silvestri simone-silvestri left review comments

@glwagner glwagner glwagner approved these changes

@navidcy navidcy navidcy approved these changes

Assignees
No one assigned
Labels
documentation 📜 The sacred scrolls user interface/experience 💻
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@iuryt @glwagner @navidcy @simone-silvestri