fims::Vector operators and implicit scalar conversion #602
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
by cleaning up the R code
Removes functions
* mkObj
* create_rcpp_interface_object
* use_module
* %>%
* FIMSFrameAge
Adds importFrom(TMB, MakeADFun) to package b/c TMB is no longer
explicitly used in the codebase. And, experimented with removing usethis
b/c it was also used in ./R/ but is still used in data-raw. It is now a
suggested package. Uses :: instead of importing functions, e.g.,
utils::head().
* Constricts line width to 80 characters
* Use full stops at the end of sentences
* Implement more spacing in between roxygen tags
* Move export tag to the end in most instances
* Use explicit brackets for if statements
* Remove duplicated code in gtest by calling run_gtest()
* Parameter and object names to snake_case
* Parameter, function, and object names, largely, w/o abbreviations
fix(run_gtest): Use paste() instead of paste0() to ensure proper spacing
which is not explicit in the instructions. With paste()
they can input as many additional arguments as they want and they are
all automatically separated, e.g.,
`run_gtest("--something", "--something-else")`
Close #588
Close #564
Close #590
Instructions for code reviewerHello reviewer, thanks for taking the time to review this PR!
Checklist
|
|
I think it might be better to just rebase the proto branch with main rather than merging commits into it, |
- document units in code snippets of the fims-demo vignette - add units to output figures in the fims-demo vignette and add a spawning biomass comparison figure - add an R test to confirm that age composition intput can be in numbers or proportions - add units for index_data and age_data in data_mile1.R Co-authored-by: Kelli Johnson <kelli.johnson@noaa.gov>
This is necessary because changes were made to codecov.io so that a token is required to upload.
|
@msupernaw do we need to be doing something to move this PR along or did you rebase instead, which would allow this PR to be deleted? |
|
I have a comment in our developer's notes that we decided not to implement implicit conversion to keep our code honest. Can you remind me the pros of implicit conversion? If we do move forward with this change, I agree with Kelli that this branch should be rebased either with main or dev. |
- post documentation for users, developers, and project managers - reorganize vignettes under appropriate sectionss - remove the navbar for articles to ensure no articles are hidden
* Fixes the triage label to "status: triage_needed" * Adds text on where to find documentation of reprex and that you should use one. Copied from the dplyr issue template. Close NOAA-FIMS/collaborative_workflow#136
We no longer import the magrittr pipe
For catch and index data the measurement of uncertainty should be the standard deviation (sd) of the logged observations. The operating model outputs a CV. I added a temporary function in data_mile1.R to calculate the sd from the CV. In the future we will need to, behind the scenes, take the log of the sd so the parameters stay within the bounds, which is what the vignette is doing now. This time series of uncertainty can later be fed to the modules but I did not change the vignette. Close #136
For users and developers, where the information for the latter was moved to the collaborative workflow see NOAA-FIMS/collaborative_workflow#146. A badge for R-universe was placed in the list of all badges! Work was done by @k-doering-NOAA with suggestions by @kellijohnson-NOAA Part of NOAA-FIMS/collaborative_workflow#98
They have been moved and updated in NOAA-FIMS/.github/.github with NOAA-FIMS/.github@33c261d. Now they are available to all repositories not just NOAA-FIMS/FIMS. I did not migrate over the refactor_request issue template because it was largely redundant with the developer_other_issue template and I thought it was not needed.
|
@msupernaw given that we are not going to use implicit conversion, @Andrea-Havron-NOAA is asking what are the "pros of implicit conversion"? Once we answer that, can we delete this PR? |
|
The pro is that we can use a fims::Vector to represent scalar values. The
implicit conversion returns the first index of the vector if the vector is
of size 1.
//Test operator "=" a scalar
*fims::Vector<double>* pi;
pi = 3.14159265359;
EXPECT_NEAR(pi, 3.14159265359, 0.000001);
//Test implicit conversion to a scalar
* double pi_scalar = pi*;
EXPECT_EQ(pi_scalar, pi[0]);
https://github.com/NOAA-FIMS/FIMS/blob/proto_fims_vector_operators/tests/gtest/test_fims_vector_operators.cpp#L46
|
Co-authored-by: Cole-Monnahan-NOAA <Cole-Monnahan-NOAA@users.noreply.github.com>
and check column names in FIMSFrame The column names of the S4@data object were being checked with the validation function but there was no check in FIMSFrame and to do the calculations within FIMSFrame all of the columns need to be present. Right now we are checking it twice, once in FIMSFrame and once in the validator of the FIMSFrame class. We might need a different class like an input data class and we could check the class upon input to FIMSFrame but this works for right now. @k-doering-NOAA do you have any ideas here? Had to change the test of a data frame without the proper columns to just error out because no warnings are given just a stop() command. When writing a data frame to a csv and reading it back in, the date formatting can be lost, e.g., 0001-01-01 turns into 1-1-1. FIMS requires a yyyy-mm-dd format. Now the use of the as.Date() function will create a date object from a character object but only if it is in the correct format. If it is in the wrong format, e.g., yyyy/mm/dd, then the function will error out. Right now the start_year and end_year are formatted as integers because for plotting, I thought it would be better to have year 1 versus year 0001 but we can change this. @ian-taylor-NOAA what do you think? Close #639 --------- Co-authored-by: Kelli.Johnson <1536491690113302@mil>
|
@msupernaw now that parametervectors are in dev can we delete this PR and the proto_fims_vector_operator branch? |
|
hmm, I think it may be good to at least preserve the proto_operators branch
somewhere. We may decide to use that code sometime in the future.
…On Wed, Oct 9, 2024 at 11:57 PM Kelli Johnson ***@***.***> wrote:
@msupernaw <https://github.com/msupernaw> now that parametervectors are
in dev can we delete this PR and the proto_fims_vector_operator branch?
—
Reply to this email directly, view it on GitHub
<#602 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABFUSEC7EQZFWSXQKYTU5MTZ2X3LNAVCNFSM6AAAAABINIIFPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMBTHA4TOMZTGY>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Matthew Supernaw
Mathematical Statistician
*National Oceanic and Atmospheric Administration*
*Office Of Science and Technology*
*NOAA Fisheries | *U.S. Department of Commerce
Phone 248 - 396 - 7797
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What is the feature?
How have you implemented the solution?
Does the PR impact any other area of the project?
How to test this change
Developer pre-PR checklist