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

Outputs comparison suite data to json and AtmosCO2 #129

Merged
merged 16 commits into from
Apr 30, 2024
Merged

Outputs comparison suite data to json and AtmosCO2 #129

merged 16 commits into from
Apr 30, 2024

Conversation

ledm
Copy link
Collaborator

@ledm ledm commented Apr 19, 2024

Two major changes:

  1. A new function that output time series data as a json file, which can then be easily exported outside bgcval (or even out of python).
  2. A new field for atmospheric CO2, AtmosCO2, is now included in bgval2 by default, as TF are all running in emissions.

@ledm
Copy link
Collaborator Author

ledm commented Apr 19, 2024

The tests arefailing because I added a new import: jsondiff. @valeriupredoi, how do I fix that?

@valeriupredoi
Copy link
Owner

this the package you added? https://anaconda.org/conda-forge/jsondiff

@ledm
Copy link
Collaborator Author

ledm commented Apr 19, 2024

Looks like that works, good job V.

@valeriupredoi
Copy link
Owner

no failed test can ever stand a chance against me, bud 😁

details['dpi'] = input_yml_dict.get('dpi', None)
details['savepdf'] = input_yml_dict.get('savepdf', False)
details['savecsv'] = input_yml_dict.get('savecsv', False)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Probably worth adding this functionality into documentation.

Copy link
Owner

Choose a reason for hiding this comment

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

yes!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thinking about it, maybe it should be savejson, instead of savecsv?

Copy link
Owner

Choose a reason for hiding this comment

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

no IMHO since json is just the transport/payload, CSV is the final format no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The final files are json format. It's basically a human readable python dictionary. Kinda like a shelve file that isn't a weird format.

Copy link
Owner

Choose a reason for hiding this comment

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

human-readable and json are not sitting well on the same bus, bud 😆

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Too late! Changed it to json!

Copy link
Owner

Choose a reason for hiding this comment

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

whatever you think best, man - am merely devops here 😀

@ledm
Copy link
Collaborator Author

ledm commented Apr 24, 2024

Ready for review @valeriupredoi!

@valeriupredoi
Copy link
Owner

Ready for review @valeriupredoi!

on it right now, buds 🍺

Copy link
Owner

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

looking good: remember about some docs (in README methinks) 🍺

title=title,
ts=ts,
csvFolder=csvFolder)

Copy link
Owner

Choose a reason for hiding this comment

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

gets the prize for the function with most number of args I've seen 😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

image


"""
if csvformat.lower() not in ['json', '.json']:
print('Format not set up', csvformat )
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
print('Format not set up', csvformat )
print('Looking for a json or .json file extension. Format not set up', csvformat )

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

or

raise TypeError

?

Copy link
Owner

Choose a reason for hiding this comment

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

yeah if you need it to exit right away, maybe OSError instead? TypeError refers to a type of a Python(-already) object that can not be operated on

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changed

bgcval2/timeseries/timeseriesTools.py Show resolved Hide resolved
@ledm
Copy link
Collaborator Author

ledm commented Apr 30, 2024

Okay - all good here. Happy to merge @valeriupredoi.

Copy link
Owner

@valeriupredoi valeriupredoi left a comment

Choose a reason for hiding this comment

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

go for it, buds 🍻

@ledm ledm merged commit 7defacb into main Apr 30, 2024
1 check passed
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