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

Time log based on new update #635

Merged
merged 5 commits into from
Aug 21, 2023

Conversation

AminTorabi-NOAA
Copy link
Contributor

Time log is added that record the time for creating a network, forcing, run time, data assimilation and at the end output the results.

Additions

Removals

Changes

Testing

Screenshots

Notes

Todos

Checklist

  • PR has an informative and human-readable title
  • Changes are limited to a single goal (no scope creep)
  • Code can be automatically merged (no conflicts)
  • Code follows project standards (link if applicable)
  • Passes all existing automated tests
  • Any change in functionality is tested
  • New functions are documented (with a description, list of inputs, and expected output)
  • Placeholder code is flagged / future todos are captured in comments
  • Visually tested in supported browsers and devices (see checklist below 👇)
  • Project documentation has been updated (including the "Unreleased" section of the CHANGELOG)
  • Reviewers requested with the Reviewers tool ➡️

Testing checklist

Target Environment support

  • Windows
  • Linux
  • Browser

Accessibility

  • Keyboard friendly
  • Screen reader friendly

Other

  • Is useable without CSS
  • Is useable without JS
  • Flexible from small to large screens
  • No linting errors or warnings
  • JavaScript tests are passing

@shorvath-noaa
Copy link
Contributor

@AminTorabi-NOAA It looks like this new version is missing self._log_parameters as an output from _read_config_file(bmi_cfg_file)

@shorvath-noaa
Copy link
Contributor

@AminTorabi-NOAA I forget if we were going to add a 'print timing' option in the finalize() function of bmi_troute.py?

@AminTorabi-NOAA
Copy link
Contributor Author

@shorvath-noaa Yes, that was my understanding since other functions might be used in a loop, we can't simply add a print statement at the end of that function. Let me know if you want me to change the structure

@AminTorabi-NOAA
Copy link
Contributor Author

@AminTorabi-NOAA It looks like this new version is missing self._log_parameters as an output from _read_config_file(bmi_cfg_file)

Oh thanks, I missed that. Let me know if there is any other comments. I'll address all together.

@shorvath-noaa
Copy link
Contributor

@AminTorabi-NOAA Can you make those two changes to start with, particularly the _log_parameters? I can't test the rest of it until that is fixed. It would also be nice to go ahead an incorporate the edit to the finalize() function in bmi_troute.py



def finalize(self,):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we rename this function? Something like print_timing_summary()?

time_value = self.task_times[key]
percentage = (time_value / total_time) * 100
print(
f'{key} construction: {time_value:.2f} secs, {percentage:.2f} %'
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you update the print statement with specific text rather than using the task_times.keys()? Take a look at the print statements in main.py, but something like "Network graph construction: ..." rather than "network_time construction: ..."

percentage = (time_value / total_time) * 100
print(
f'{key} construction: {time_value:.2f} secs, {percentage:.2f} %'
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add another print('---') type line and then print the total run time?

Copy link
Contributor

@shorvath-noaa shorvath-noaa left a comment

Choose a reason for hiding this comment

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

Nice work!

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