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

Release 4.0 append output for save_results #1225

Open
wants to merge 6 commits into
base: release-4.0
Choose a base branch
from

Conversation

afinit
Copy link
Collaborator

@afinit afinit commented Jul 8, 2023

Describe your changes

I expect changes, but this seems like a good start for discussion.

  • adds error handling for an illegal outformat param
  • adds append param that works for both json and csv. defaults to True
  • changes the code to use with to open csv file

Before this PR, neither json nor csv files would append data. The json path reads in the existing data, but throws away the observations and only keeps the metadata.

The part that needs some discussion:
With this PR, existing json and csv data are treated differently. When json data is appended, we overwrite any observations that have the same sample name as our new data. When csv data is appended, we are strictly appending without any consideration of the data that is already in the file.

I don't like this difference in behavior

Options for fixing the differences

There are probably other options, but these are the 2 ideas I came up with to resolve the current difference in behavior

Option 1

Read existing CSV data with pandas and remove any rows with the same sample name as the new data

Option 2

Read existing data for both json and csv. Modify conflicting sample names to have an appended _### where we autoincrement that number for new data. This would look something like:

# existing data:
sample,trait,value,label
default,string,string,none
default_001,boolean,1,none

# new data:
sample,trait,value,label
default,string,string2,none
default,boolean,0,none

# data written:
sample,trait,value,label
default,string,string,none
default_001,boolean,1,none
default_002,string,string2,none
default_002,boolean,0,none

Type of update

new feature

Associated issues

Addresses #886

For the reviewer

See this page for instructions on how to review the pull request.

  • PR functionality reviewed in a Jupyter Notebook
  • All tests pass
  • Test coverage remains 100%
  • Documentation tested
  • New documentation pages added to plantcv/mkdocs.yml
  • Changes to function input/output signatures added to updating.md
  • Code reviewed
  • PR approved

@codecov
Copy link

codecov bot commented Jul 8, 2023

Codecov Report

Merging #1225 (4b2d1b9) into release-4.0 (381cf6f) will not change coverage.
Report is 2 commits behind head on release-4.0.
The diff coverage is 100.00%.

Impacted file tree graph

@@              Coverage Diff              @@
##           release-4.0     #1225   +/-   ##
=============================================
  Coverage       100.00%   100.00%           
=============================================
  Files              157       157           
  Lines             6759      6767    +8     
=============================================
+ Hits              6759      6767    +8     
Flag Coverage Δ
unittests 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
plantcv/plantcv/classes.py 100.00% <100.00%> (ø)

@nfahlgren
Copy link
Member

@all-contributors please add @afinit for code, doc, test.

@allcontributors
Copy link
Contributor

@nfahlgren

I've put up a pull request to add @afinit! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants