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

better description for TimeMonitor.interval #1187

Merged
merged 1 commit into from
Oct 2, 2023

Conversation

tylerflex
Copy link
Collaborator

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

This looks good, but while we're at it I wonder if we should change the default. People tend to have very big time monitors without realizing... But I'm not sure what's a good default setting. One thing could be the nyquist sampling rate used in the frequency monitors, which is on the order of 10-20 typically. But I don't think we want to not have a clear default (i.e. to be automatically reset by the simulation). Or do we? Or can we change defaults in some other way, e.g. not have start and stop time be beginning and end of the full simulation?

@tylerflex
Copy link
Collaborator Author

I don't think we should change defaults just because it will mess with people's existing scripts.

One thing I could imagine though is setting the default value to None and warning in a validator that:

  • we will set interval will be 1 by default.
  • this can lead to a lot of data.
  • if you really wanted this, to get rid of this warning explicitly set interval=1 in your monitor.

@momchil-flex
Copy link
Collaborator

I think that sounds good! You wanna do this as part of this PR?

@tylerflex
Copy link
Collaborator Author

ok. what do you recommend for a good recommended interval value?

@momchil-flex
Copy link
Collaborator

I feel like maybe we shouldn't recommend a value; just print the warning if interval=None and also start=0 and stop=None (the defaults). Just say that this may produce a lot of data, and consider using these arguments to reduce that.

@tylerflex tylerflex force-pushed the tyler/fix/monitr_interval_description branch from 7e31a43 to 73f74e0 Compare October 2, 2023 13:35
@tylerflex tylerflex force-pushed the tyler/fix/monitr_interval_description branch from 73f74e0 to 1339c45 Compare October 2, 2023 13:38
@momchil-flex momchil-flex merged commit 56b4603 into develop Oct 2, 2023
16 checks passed
@momchil-flex momchil-flex deleted the tyler/fix/monitr_interval_description branch October 2, 2023 16:26
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

Successfully merging this pull request may close these issues.

2 participants