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

Splitting "big box" and "little box" interfaces apart (attempt #2) #384

Merged
merged 7 commits into from
Oct 10, 2023

Conversation

JosephCottam
Copy link
Contributor

Moves the big-box interfaces into their own module and deprecates them.

@JosephCottam JosephCottam added WIP PR submitter still making changes, not ready for review integration Tasks for integration with TA4 labels Oct 3, 2023
@JosephCottam JosephCottam self-assigned this Oct 3, 2023
@JosephCottam JosephCottam linked an issue Oct 3, 2023 that may be closed by this pull request
@JosephCottam
Copy link
Contributor Author

@SamWitty The failures on these tests occur on my own machine, but only sometimes. Can you look and let me know if I did something wrong in my translation to small-box interfaces?

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@JosephCottam
Copy link
Contributor Author

@SamWitty I have the notebooks executing with the exception of one cell.

Here are notes on the choices I made:

  1. The default start-state passed into setup_model was derived by the big-box function. I moved that derivation into setup_model.
  2. I did not understand the 'autoguide' stuff being done, so I kept it in the notebook. This may not be reasonable in the long-run because it requires constructing a thing from the model to pass into another part of the interface BUT I don't know when to derive what (or how). We might take a 'wait and see if its a problem' approach for this one.
  3. The last cell of 'demo_ensemble' notebook does not succeed where it used to. Apparently it used to get 'backcast' entries but now it does not. I'm not sure why it doesn't anymore.

Depending on how important these things are to you (esp. 2 and 3), this might be ready for merge.

@JosephCottam JosephCottam removed the WIP PR submitter still making changes, not ready for review label Oct 5, 2023
Copy link
Contributor

@SamWitty SamWitty left a comment

Choose a reason for hiding this comment

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

Looks good. There are a few minor refactors I'd like to make, but I'll add those as a separate PR.

@SamWitty SamWitty merged commit 1f9b902 into main Oct 10, 2023
3 checks passed
@JosephCottam JosephCottam deleted the small-box-interfaces2 branch November 1, 2023 23:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integration Tasks for integration with TA4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Create "small-box" interface version
2 participants