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

Building path using os library instead of f-strings for real time analysis #616

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

ijpulidos
Copy link
Contributor

@ijpulidos ijpulidos commented Aug 2, 2022

Description

Resolves #615 by using the os.path.join method instead of relying on manually introduced f-strings.

Todos

  • Implement feature / fix bug
  • Add tests
  • Update changelog to summarize changes in behavior, enhancements, and bugfixes implemented in this PR

Status

  • Ready to go

Changelog

Bug in generating real time analysis output when reporter is in the same working directory. Fixed by using ``os.path.join`` for creating the output path.

@ijpulidos ijpulidos added this to the 0.21.5 milestone Aug 2, 2022
@ijpulidos
Copy link
Contributor Author

It might seem trivial, but does it make sense to create a test for this? It can be something that checks running some short simulation and outputs the real time analysis file in the current working directory.

@ijpulidos ijpulidos requested a review from mikemhenry August 2, 2022 01:52
@zhang-ivy
Copy link
Contributor

Previously, I was having trouble when feeding a file with no directory to the reporter, e.g. analysis.yaml. I had to change this to be ./analysis.yaml, but I think we should make sure that specifying only a file works as well. Can we make sure this PR allows this?

@ijpulidos
Copy link
Contributor Author

ijpulidos commented Aug 2, 2022

@zhang-ivy Yes, I guess you mean specifying output.nc or similar when instantiating the reporter. And yes, this is exactly the problem that this solves, when you don't specify a directory for the reporter its directory attribute becomes an empty string and that is fine with os.path.join. That is, os.path.join('', 'output.nc') returns 'output.nc' as expected.

@zhang-ivy
Copy link
Contributor

Great!

Copy link
Contributor

@mikemhenry mikemhenry left a comment

Choose a reason for hiding this comment

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

LGTM!
For the refactor, we should try and use pathlib for everything

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.

Realtime analysis output failing when output has no subdirectory
3 participants