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

adapt to refactored sodym visualization and data writing #10

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JakobBD
Copy link
Collaborator

@JakobBD JakobBD commented Oct 10, 2024

Plastics is tested, steel fails due to missing data in the data repo. Please update @Merjo!

@JakobBD JakobBD requested review from SallyDa and Merjo October 10, 2024 15:35
Copy link
Collaborator

@SallyDa SallyDa left a comment

Choose a reason for hiding this comment

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

Looks good to me :)
One more thing you could think about is making a SankeyConfig to go here, so that it is clear what is mandatory and what can optionally be included under sankey in the .yml file.

from sodym.export.data_writer import DataWriter
from sodym.export.helper import ArrayPlotter
from sodym.export.data_writer import export_mfa_flows_to_csv, export_mfa_stocks_to_csv, export_mfa_to_pickle
from sodym.export.array_plotter import PlotlyArrayPlotter
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could think about adding an __init__.py file to the sodym/export directory, to simplify these imports, so that we could do from sodym.export import PlotlyArrayPlotter, SankeyArrayPlotter. I did this on the sodym directory level, but tbh, it would be good check everything is there that we want to be there. And clearly this is not a necessary thing, but can make the imports look a bit nicer (and has the advantage that users do not need to know the file names within the sodym/export module).

@SallyDa
Copy link
Collaborator

SallyDa commented Oct 11, 2024

Btw, I thought some of the steel data was propriety, and this is why it has not been added to git? I don't have that data locally either, so I have never been able to run the steel model. Not sure how you would feel about making the package private and adding the data to git? Scientific reproducibility anyway goes out the window if the data is not accessible. I'm not sure I really want to make the code inaccessible either, but maybe it's worth considering to ease our development.

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