nc-minibbs / mbbs

A repository for the Mini-Bird Breeding Survey data
https://minibbs.us
Other
2 stars 0 forks source link

Observers #36

Closed IJBG closed 10 months ago

IJBG commented 1 year ago

Just about ready to integrate this into the overall workflow

@bsaul looking at the process_observers.R script, do you want it located somewhere other than R/? Are the function headers formatted correctly, and do I need to run roxygen script of some kind to generate the help pages that show up in the man/ folder?

bsaul commented 1 year ago

do you want it located somewhere other than R/?

R/ is good.

Are the function headers formatted correctly, and do I need to run roxygen script of some kind to generate the help pages that show up in the man/ folder?

If you're using Rstudio, Cmd + Shift + D is (I think the standard shortcut for building documentation using roxygen. There's also a setting (or there used to be) to build the documentation when you build the package (Cmd + Shift + B is the shortcut for devtools::build() I think).

Also, be sure to devtools::check() the package. I think the shortcut is Cmd + Shift + C.

IJBG commented 1 year ago

Updated the csv files scrapped from the old website with the observers Haven sent over - seems to have changed the date format in Orange from ymd to mdy, should I leave in a comment about this in the import_data file? ie: # NOTE: date format is different in orange county data date = lubridate::mdy(date), #date format here WAS ymd before time = as.character(time) Gives an error if left as ymd

bsaul commented 1 year ago

Updated the csv files scrapped from the old website with the observers Haven sent over - seems to have changed the date format in Orange from ymd to mdy, should I leave in a comment about this in the import_data file? ie: # NOTE: date format is different in orange county data date = lubridate::mdy(date), #date format here WAS ymd before time = as.character(time) Gives an error if left as ymd

I don't think you need to leave a comment in this case. The history of the file changes is in git if we need that history for some reason..

IJBG commented 1 year ago

@bsaul Would it break anything to make the mbbs_county column uppercase? Reduce my use of toUpper + no need to re-set labels when graphing

bsaul commented 1 year ago

@bsaul Would it break anything to make the mbbs_county column uppercase? Reduce my use of toUpper + no need to re-set labels when graphing

It would break the minibbs site currently, but it wouldn't be that difficult to change.

IJBG commented 1 year ago

@bsaul Would it break anything to make the mbbs_county column uppercase? Reduce my use of toUpper + no need to re-set labels when graphing

It would break the minibbs site currently, but it wouldn't be that difficult to change.

I thought it might - would it be easier for you to use it as uppercase going forward or would you prefer we keep it lowercase? I can live with toUpper when I need it

bsaul commented 1 year ago

@bsaul Would it break anything to make the mbbs_county column uppercase? Reduce my use of toUpper + no need to re-set labels when graphing

It would break the minibbs site currently, but it wouldn't be that difficult to change.

I thought it might - would it be easier for you to use it as uppercase going forward or would you prefer we keep it lowercase? I can live with toUpper when I need it

From my perspective, it doesn't matter much (though I tend to use a convention of lowercase whenever possible so I don't have to remember to capitalize). So if it's easier for you, it's fine to change.

IJBG commented 1 year ago

From my perspective, it doesn't matter much (though I tend to use a convention of lowercase whenever possible so I don't have to remember to capitalize). So if it's easier for you, it's fine to change.

I'll mull it over and get back to you! I used 'county' rather than 'mbbs_county' in a couple places before I realized it should always be 'mbbs_county', which is where this is propagating from.

On a related track, what if we removed the 'county' column that's coming from ebird? It's a column that's theoretically at it's best an 'mbbs_county' duplicate, but it's sometimes wrong (ie: ebird thinks one of the Durham routes lat/lon starts is in Wake co.) and uses capitals. Removing it might just be totally unnecessary, pro/con the main benefit would be for first-time users, standardization of what column to use across the project (already standard in your code), and there is an option for already-capital names. Cons, the Wake/Durham issue can be fixed with a quick addition to importing Durham data in import_data, and nothing that uses county would break anywhere.

bsaul commented 1 year ago

On a related track, what if we removed the 'county' column that's coming from ebird?

I'm for this, as I agree that's it's confusing.

IJBG commented 11 months ago

@bsaul Do you know how to resolve this merge conflict? I suspect it's because I got rid of the county column. Branch is all ready to merge :)

bsaul commented 11 months ago

@bsaul Do you know how to resolve this merge conflict? I suspect it's because I got rid of the county column. Branch is all ready to merge :)

Yeah, hmmm. Do you try the command-line instructions that github provides? If that doesn't work we may need to some sort of forced merge.