search

AquaSat / AquaMatch_harmonize_WQP

https://aquasat.github.io/AquaMatch_harmonize_WQP/
MIT License
1 stars 3 forks source link

small tweaks to chla/ASv2 code #63

Closed steeleb closed 11 months ago

steeleb commented 11 months ago

Just two small tweaks (but looks like more, since the WQP is ever-changing).

1) adds required packages, installs in 'run.R' - I think there are some hidden package dependencies within the code from USGS - like dataRetrieval is never actually listed as a necessary package in the 2_download.R script. I honestly don't understand how it works without citing those packages because targets is so particular about that... MB, please explain! 🤯🤣

2) adds .feather files to the .gitignore

3) a bunch of other minor changes to the outputs, since, as stated, the WQP is ever changing.

WHEEEE!

steeleb commented 11 months ago

Also, it looks like there are a bunch of weird changes due to the fact that there aren't TSS or DOC data present in the current pull - MB, this might actually require your attention. I'm not sure why that is happening. If it makes sense for me to only commit those two files, please let me know and I'll delete and make a new PR.

mbrousil commented 11 months ago

Also, it looks like there are a bunch of weird changes due to the fact that there aren't TSS or DOC data present in the current pull - MB, this might actually require your attention. I'm not sure why that is happening. If it makes sense for me to only commit those two files, please let me know and I'll delete and make a new PR.

Ooooh ok yeah would you mind doing that? I think part of it is also that I haven't been committing new versions of all of the small files with each run - I've mostly focused just on the scripts for now to avoid overwhelming you all in the PRs. So some of the files changed look like stuff I have locally after running the full pipeline, the outputs of which just haven't been committed yet.

steeleb commented 11 months ago

@mbrousil OH! Given that information, I actually think we should do one of the two things: 1) add the files that you don't want to track to the .gitignore, or 2) let this be the update to the repo. Otherwise, the files aren't representative of the actual pipeline, which is not good, IMO.

Are there items in this PR that we shouldn't be tracking?

mbrousil commented 11 months ago

The only thing I've been avoiding committing is stuff in chapters/, but I'm not sure that there's a good reason not to. I basically wanted to put up _book/ as a priority and chapters/ felt redundant. But we can add it in if you'd prefer.

mbrousil commented 11 months ago

Hmm and _targets/, based on this section in the {targets} user manual. That's something I've been wondering about though, cuz I know you do it differently

steeleb commented 11 months ago

Hmm and _targets/, based on this section in the {targets} user manual. That's something I've been wondering about though, cuz I know you do it differently

There's an additional .gitignore that is automatically populated with targets that lives in the _targets folder, so it doesn't track objects, just the meta/, but I'll be honest that I don't see a reason to track anything in that folder, rather was defaulting to {targets} default. Definitely worth coming up with a plan for this, though I don't think we do it differently.

The only thing I've been avoiding committing is stuff in chapters/, but I'm not sure that there's a good reason not to. I basically wanted to put up _book/ as a priority and chapters/ felt redundant. But we can add it in if you'd prefer.

I'm more concerned about the large changes in files that I would have expected to be tracked (in /out/ folders, for instance). I don't have any strong feelings about the _book versus chapters, I think those are holdovers from the {targets} pipeline. If the chapters folder is redundant, however, that should be added to the .gitignore.

mbrousil commented 11 months ago

On review, I think it would make sense to commit basically everything that's included here. I'm leaning towards having you do the PR for the two files you mentioned, then I would re-run the pipeline and commit the freshly updated files out of the finished pipeline. That way nothing is accidentally left out from my local run. Do you think that makes sense?

steeleb commented 11 months ago

I think that's a fine way to move forward, with the acknowledgement that it will impact @kathryn-willi's ability to move forward with review. If it's okay that she doesn't look at this until after break, then I say we close this PR and I'll submit another with only the changes to .gitignore and run.R.

mbrousil commented 11 months ago

Yep, that's fine with me.