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.

Sign up for GitHub

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

Jump to bottom

demo vignette modification suggestions #705

Merged
merged 9 commits into from
Dec 18, 2024
Merged

demo vignette modification suggestions #705

merged 9 commits into from
Dec 18, 2024

Conversation

k-doering-NOAA
Copy link
Member

What is the feature?

  • Small changes to the vignette

How have you implemented the solution?

  • Committed small grammatical/spelling/clarity edits
  • One suggestion not yet done: I see the @data slot is accessed directly, which I thought was not supposed to be how users access slots; instead, I think the FIMS::get_data() function should be used.
    According to advanced R on accessors,

Slots should be considered an internal implementation detail: they can change without warning and user code should avoid accessing them directly. Instead, all user-accessible slots should be accompanied by a pair of accessors.

I can make this change, if we agree that it should be done!

Does the PR impact any other area of the project, maybe another repo?

  • no

@github-actions GitHub Actions
Copy link
Contributor

Instructions for code reviewer

Hello reviewer, thanks for taking the time to review this PR!

  • Please use this checklist during your review, checking off items that you have verified are complete!
  • For PRs that don't make changes to code (e.g., changes to README.md or Github actions workflows), feel free to skip over items on the checklist that are not relevant. Remember it is still important to do a thorough review.
  • Then, comment on the pull request with your review indicating where you have questions or changes need to be made before merging.
  • Remember to review every line of code you’ve been asked to review, look at the context, make sure you’re improving code health, and compliment developers on good things that they do.
  • PR reviews are a great way to learn, so feel free to share your tips and tricks. However, for optional changes (i.e., not required for merging), please include nit: (for nitpicking) before making the suggestion. For example, nit: I prefer using a data.frame() instead of a matrix because...
  • Engage with the developer when they respond to comments and check off additional boxes as they become complete so the PR can be merged in when all the tasks are fulfilled. Make it clear when this has been reached by commenting on the PR with something like This PR is now ready to be merged, no changes needed.

Checklist

  • The PR is requested to be merged into the appropriate branch (typically main)
  • The code is well-designed.
  • The functionality is good for the users of the code.
  • Any User Interface changes are sensible and look good.
  • The code isn’t more complex than it needs to be.
  • Code coverage remains high, indicating the new code is tested.
  • The developer used clear names for everything.
  • Comments are clear and useful, and mostly explain why instead of what.
  • Code is appropriately documented (doxygen and roxygen).

Bai.Li and others added 6 commits December 17, 2024 11:12
@kellijohnson-NOAA
updates wrapper functions to use length data
b6a77b5 
* update initialize_modules(), initialize_fims(), and initialize_fleet()
  to use length data
* add data_mile1_add_length.R to set up FIMSFrame object with length
  data and change to data1
* use dplyr::pull() instead of anonymous function
* use function n_years(data) instead of accessing n_years slot from data
  (e.g., data@n_years)
* updates integration tests
* Part of #702 for changing data_mile1 to data1
@kellijohnson-NOAA @Andrea-Havron-NOAA
@Bai-Li-NOAA @Cole-Monnahan-NOAA @JaneSullivan-NOAA @nathanvaughan-NOAA
Updates documentation of C++ for Rcpp
e58b3f1 
Fully documents Parameter and ParameterVector matching documentation at the
function definition with what is shown in Rcpp methods::show() inside R.

Co-authored-by: @Andrea-Havron-NOAA <[email protected]>
Co-authored-by: @Bai-Li-NOAA <[email protected]>
Co-authored-by: @Cole-Monnahan-NOAA <[email protected]>
Co-authored-by: @JaneSullivan-NOAA <[email protected]>
Co-authored-by: @nathanvaughan-NOAA <[email protected]>
@kellijohnson-NOAA
fix(FIMSFrame setValidity): Uses object@data instead of data1
89d9e34
@kellijohnson-NOAA
fix(Parameter): Removes estimated_m
13804ac 
because we have is_random_effect
@kellijohnson-NOAA
rm(SetFIMSFunctios): Removes unused function
30436ae
@k-doering-NOAA @kellijohnson-NOAA
fix(fims-demo): Updates vignette links
34601e8 
* doi to Bai et al.
* fix grammar and spelling
@kellijohnson-NOAA
Copy link
Contributor

Thank you @k-doering-NOAA for this. I have since rebased this to dev-length-wrapper-and-test and we will continue to make some edits during today's (Tuesday 2024-12-17) office hour. After which, we can merge this into dev. During the office hour we will focus on the accessor functions of FIMSFrame.

kellijohnson-NOAA and others added 3 commits December 17, 2024 21:23
@kellijohnson-NOAA @JaneSullivan-NOAA
@Andrea-Havron-NOAA @nathanvaughan-NOAA @peterkuriyama-NOAA
refactor(get_*, m_*) feat(get_*): Accessors
773c623 
Refactor, increase, and overhaul accessors for FIMSFrame and FIMSFit!
Changes to use get_*() and m_*() for all accessors where it was not
consistent before. Also changed to using a single file to document all
of the accessors that can be grouped together.

Added a ton of documentation, i.e., R comments, to the fimsframe and
fimsfit files so it is more clear how to add to them if you are a new
developer.

Increased the consistency of naming and documentation styles, added
VS Code/RStudio style headers to the two files to help with navigation.

Deleted weight_at_age slot from FIMSFrame because m_weight_at_age should
be used instead of getting that information from the slot.

HUGE THANKS to those that showed up and helped with this large task
during the FIMS Office Hour today :)

Co-authored-by: @JaneSullivan-NOAA <[email protected]>
Co-authored-by: @Andrea-Havron-NOAA <[email protected]>
Co-authored-by: @nathanvaughan-NOAA <[email protected]>
Co-authored-by: @peterkuriyama-NOAA <[email protected]>
@kellijohnson-NOAA
refactor(Rcpp::Rcout): Adds () and spaces to print out
569c590
@kellijohnson-NOAA
refactor: Remove code that is commented out
0b952b1
@kellijohnson-NOAA
Copy link
Contributor

We rocked it during the FIMS Office Hour today! We were able to clean up, document, and add to the R/fimsframe.R and R/fitfims.R files. I really cannot believe how much we got done. I restructured a few things this afternoon after the work session because I realized that it would be better to document all get_() functions in R/fimsframe.R in their own file and likewise for m_(). Additionally, the get_*() functions from R/fimsfit.R are also in a single file now. Then I knocked off a few trivial tasks from #662 like removing code that was commented out and fixing a few things being printed to the screen by Rcpp::Rcout().

@kellijohnson-NOAA kellijohnson-NOAA added kind: documentation Improvements or additions to documentation kind: refactor Restructure code to improve the implementation of FIMS theme: code cleanup theme: documentation labels Dec 18, 2024
@kellijohnson-NOAA kellijohnson-NOAA added this to the Q2 milestone Dec 18, 2024
@kellijohnson-NOAA kellijohnson-NOAA merged commit cf626cf into dev Dec 18, 2024
10 checks passed
@kellijohnson-NOAA kellijohnson-NOAA deleted the dev-vignette-edits branch December 18, 2024 05:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kellijohnson-NOAA kellijohnson-NOAA Awaiting requested review from kellijohnson-NOAA

Assignees
No one assigned
Labels
kind: documentation Improvements or additions to documentation kind: refactor Restructure code to improve the implementation of FIMS theme: code cleanup theme: documentation
Projects
None yet
Milestone
Q2
Development

Successfully merging this pull request may close these issues.
2 participants
@k-doering-NOAA @kellijohnson-NOAA