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

Make parameters local #36

Closed
JordiBolibar opened this issue Feb 28, 2022 · 5 comments
Closed

Make parameters local #36

JordiBolibar opened this issue Feb 28, 2022 · 5 comments
Milestone

Comments

@JordiBolibar
Copy link
Member

Right now we still have some parameters as const global variables. This is OK for simplicity's sake for now, but it is annoying when re-running code, as the variables cannot be redefined.

This is also problematic for testing, since we want to make sure that we control the local environment of the simulation.

For all these reasons, we need to pass explicitly all the necessary variables (e.g. B or H₀) in context, in order to avoid having them as const.

@JordiBolibar
Copy link
Member Author

Partially fixed by #37. Check if more stuff can be moved to the local scope.

@JordiBolibar
Copy link
Member Author

This has been improved in #39. @facusapienza21 to check if more parameters needs to be moved to the local scope for the tests.

@JordiBolibar JordiBolibar added this to the First release milestone Mar 10, 2022
@facusapienza21
Copy link
Member

I am not sure if this problems is related to this issue, but I would like to understand why current_epoch is not currenly being updated even when the scope of current_epoch is global. Inside callback_plots we can find the update of current_epoch as

global current_epoch += 1

I am not sure where this variable is originally being created, since it seems the value specified in parameters.jl does correspond to the epoch number assigned when doing ODINN.reset_epochs(). When running the training, the REPL shows a warning message

WARNING: redefinition of constant current_epoch. This may fail, cause incorrect answers, or produce other errors.

This makes all the plots to be saved with current_epoch=1. @JordiBolibar maybe you can give me some light in this matter ;)

@JordiBolibar
Copy link
Member Author

JordiBolibar commented Oct 14, 2022

Yes, I'm aware of this. Some changes might have slipped to the main branch unwillingly. I'll try to fix this asap, but for now, basically you just have to make current_epoch a Ref in parameters.jl:

const current_epoch = Ref{Int}(1)

And then just update current epoch wherever you want in the code by doing:

global current_epoch[] += 1

The codebase is in a very fragile and experimental situation right now. As soon as we settle things up a little bit, we need to fix the tests and have them running again.

@JordiBolibar
Copy link
Member Author

This is mainly tracked now in #82, which is solved with the new Parameters subtypes, making everything local.

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

No branches or pull requests

2 participants