-
Notifications
You must be signed in to change notification settings - Fork 62
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
Enhancement/plant repr #248
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop_v3 #248 +/- ##
==============================================
- Coverage 62.89% 61.85% -1.05%
==============================================
Files 29 29
Lines 4210 4315 +105
==============================================
+ Hits 2648 2669 +21
- Misses 1562 1646 +84
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this is a great start and think it could be merged as-is. I don't want to be too nitpicky over the information printed out of the repr, as I think what you have now is a huge improvement over the old repr, and we can always add new information and features incrementally.
- For the PlantData repr, I aiways envisioned a list of assets in there, and even a little embedded map showing the locations of the turbines. I see that asset is supported in the code, is there any reason why the asset table was not included in the snippet in your pull request?
- Would be great to demonstrate the new repr function in the PlantData 101 notebook: https://openoa--248.org.readthedocs.build/en/248/examples/examplesout.html#PlantData-and-PlantMetaData-101
- Can we use pandas built-in reprs instead of using a new library (tabulate?)
from attrs import field, define | ||
from tabulate import tabulate |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to use an external library as opposed to Panda's own rendering? I believe they support ASCII rendering as print(df)
and also support rich HTML rendering in Jupyter using display(df).
- I think using Panda's built in rendering is preferable, as we'd reduce the number of OpenOA dependencies.
- It would be slick, but unnecessary, to detect which environment (Jupyter notebook vs Terminal) and render HTML if possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments, I didn't include the full listing because it was so long, but there's no reason it couldn't be shown other than it being a snippet to highlight the change in perspective. I think it'd be easy enough to list the assets (I assume by type), so that could be included. I like the idea of a really a basic plot, but that may be difficult to get a consistent render between the terminal and a Jupyter notebook.
I fully intend to integrate it into the intro notebook once the form is more or less final.
As for tabulate, this is already a Pandas dependency for some of their rendering, and so we're not getting rid of it anytime soon, whichever way we ultimately go. The decision to use it directly, and in place of Pandas, is to have a consistent data view in a terminal and a jupyter notebook, something Pandas doesn't do well out of the box. I tried to also avoid the slick solutions because they're typically harder to maintain over time, but a quick search seems to suggest might be as simple as is_terminal = sys.stderr.isatty()
and when True, just do normal pandas printing, and if False, print the text rendering.
@jordanperr, the latest update was a little tricky to manage, so I decided to add a markdown method that can be called, like the below. plant.markdown() PlantData analysis_type
asset
scada
meter
tower no data status no data curtail
reanalysis
merra2
The other notebook option is to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Impressive work! The pretty printing is a really nice addition to the code. This looks good to me.
This PR creates a pretty printed version of the
PlantData
andPlantMetaData
objects, to offer a better visual representation of the data contained in each.A snippet of the new
PlantData
output can be seen below.And, a snippet of the new
PlantMetaData
output can seen below.Resolves #242 for the vast majority of use cases. @ejsimley, do you think this addresses the legibility of the underlying data, or would some other rendering be more helpful? Now that I have the scaffolding set up, changing what the displayed data look like is fairly easy.