485 tadacomparabledataidentifier is used for legend labels in tada twocharacteristicscatterplot uses a different column for id cols - Githubissues

USEPA / EPATADA

This R package can be used to compile and evaluate Water Quality Portal (WQP) data for samples collected from surface water monitoring sites on streams and lakes. It can be used to create applications that support water quality programs and help states, tribes, and other stakeholders efficiently analyze the data.

https://usepa.github.io/EPATADA/

Creative Commons Zero v1.0 Universal

40 stars 18 forks source link

485 tadacomparabledataidentifier is used for legend labels in tada twocharacteristicscatterplot uses a different column for id cols #490

Closed wokenny13 closed 2 months ago

wokenny13 commented 2 months ago

Current progress on addressing the legend labels in TADA_TwoCharacteristicScatterplot. See previous comments for details.

hillarymarler commented 2 months ago

@wokenny13 - it looks like there are some check failures related to use of this function in at least one vignette? Do you want to try and troubleshoot that before I review? Or do you want me to look into it/correct it as part of my review?

wokenny13 commented 2 months ago

@hillarymarler I think it would be good practice for me to try to troubleshoot the issues found in the checks on my own.

However, do you think it would be appropriate for you to review the functionality of the TADA_TwoCharacteristicScatterplot updates, to determine if it functions appropriately, or if the updated arguments to the function should be included/excluded, prior to updating any vignette with changes that were made to the TADA_TwoCharacteristicScatterplot first?

-Or should a pull request only be made if all checks can be passed first?

hillarymarler commented 2 months ago

@wokenny13 - I can review the function updates while you are working on check issues related to vignette(s). I'll take a look either later this afternoon or first thing tomorrow morning and we can discuss during our working session tomorrow.

wokenny13 commented 2 months ago

I made some edits that are ready for review @hillarymarler

wokenny13 commented 2 months ago

Made a draft function for handling NAs for figure titles within this pull request.

I have linked that issue to this pull request in here.

hillarymarler commented 2 months ago

Thanks for adding that new function and linking the issue. I'll review changes to scatterplot function and NA removal function tomorrow!

wokenny13 commented 2 months ago

newplot Do you think this color schema works better when 4 plots are plotted? I changed the color palette for parmC and parm D to try to make it more visually pleasing. I haven't pushed this commit though so you can view what it currently looks like and let me know your thoughts

hillarymarler commented 2 months ago

I like that color update!

wokenny13 commented 2 months ago

I've pushed the small changes through, with editing some of the coloring and remove the rm(depthcols) line. The checks have gone through.

I will keep a note, or post an issue, to consider adding a figure title for the legends in a future update for TADA.GroupedScatter. And take note to address potential TADA color palette consistency pairings.