search

NOAA-EDAB / comlandr

https://noaa-edab.github.io/comlandr/
https://noaa-edab.github.io/comlandr/
Other
0 stars 0 forks source link

In aggregate_area.R, `applyPropLand` and `applyPropValue` look like they can have different values, is this intended? #61

Open sgaichas opened 4 months ago

sgaichas commented 4 months ago

The message in the code indicates that applyPropLand and applyPropValue should not have different values (both should be T or both should be F)

However, this code allows applyPropLand to be F and applyPropValue to be T if I'm reading it correctly:

https://github.com/NOAA-EDAB/comlandr/blob/28b4c58683d35d0cdcc1b72f4b3eeda7242414a7/R/aggregate_area.R#L25-L28

Should it instead set applyPropValue to F if applyPropLand is already F?

Should line 27 be

applyPropValue <- F

And the message adjusted to say applyPropValue is being set to F?

andybeet commented 4 months ago

It isn't clear if one of the argument is T and one is F, wheher they should both be T or both be F.

We should have a check_arguments function as the first order of operations. This will prevent the user having to wait for the data pull before being told the arguments were input incorrectly.

andybeet commented 1 month ago

@slucey Any chance you can weigh in on this? Do you remember why there are two flags applyPropLand and applyPropValue? Do they both have to be T or both F?

slucey commented 1 month ago

You could easily use the same variable for both and I probably should have. The reason it was developed this way was because value is not always pulled. It is true that you would never want to proportion value and not landings. The reverse would also be true as long as you were using the values. I don't remember writing the message flag above but Sarah is right that it should set the applyPropValue to F in that case. This was sloppy coding on my part.

andybeet commented 4 weeks ago

Solution: I will recode to a single flag.

see discussion thread