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

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

Open
sgaichas opened this issue Jun 29, 2024 · 4 comments · Fixed by #78
Assignees
Labels
bug Something isn't working

Comments

@sgaichas
Copy link
Member

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:

if(applyPropLand == F & applyPropValue == T){
message("Can not apply proportions to Value and not Landings -- setting applyPropLand to F")
applyPropLand <- F
}

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
Copy link
Member

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
Copy link
Member

andybeet commented Sep 9, 2024

@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
Copy link
Member

slucey commented Sep 10, 2024

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 andybeet added the bug Something isn't working label Sep 13, 2024
@andybeet andybeet self-assigned this Sep 13, 2024
@andybeet
Copy link
Member

andybeet commented Sep 30, 2024

Solution: I will recode to a single flag.

see discussion thread

@andybeet andybeet linked a pull request Oct 1, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants