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

extract.node doc update #102

Merged
merged 5 commits into from
Feb 25, 2025
Merged

extract.node doc update #102

merged 5 commits into from
Feb 25, 2025

Conversation

andybeet
Copy link
Member

@andybeet andybeet commented Jan 31, 2025

Files updated

  • Extract.r - this contains the extract.node() function

Details

  • Updated the title and description
  • Created autolinks to other functions mentioned in the documentation
  • Expanded explanation of object output (how many variables in the list, their names, and definitions)
  • Created an example of how you would use this function using one of the bundled rpath models

NOTE: The Discard item in the returned list has duplicate field names. (See discussion 103).

I created this PR for just the one function, as a test, to make sure i was doing what you were hoping i would. Please let me know if you are expecting anything else/different

@andybeet
Copy link
Member Author

andybeet commented Jan 31, 2025

To test: switch to this branch, pull, build package, (depending on settings you may need to run devtools::document(), then run pkgdown::build_reference()).
Then navigate to the extract.node() documentation to view changes.

It should look something like this.

@andybeet andybeet mentioned this pull request Jan 31, 2025
15 tasks
@andybeet andybeet requested a review from sgaichas January 31, 2025 21:59
@andybeet
Copy link
Member Author

andybeet commented Feb 3, 2025

I think the function extract_node is working as expected but the output from rsim.run may have an issue. See discussion

#'@export

extract.node <- function(Rsim.output, group){
#Need to define variables to eliminate check() note about no visible binding
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this comment mean?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no idea

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kaydin Any ideas?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a wholly Sean function and not one that's been part of my own practical workflow. It is out-of-date in at least one way: it doesn't include fishing from input forcing (ForcedF and ForcedCatch) which e.g. is how most of the catch in our current Bering/GOA models is input. Before detail bugfixes, might be worth an overall review of this function in terms of use (if anyone on team is using this in actual workflow, would love to know).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so maybe we merge this (since it is just documentation) and then create an issue to look into it further and fix any issues. Even though the group may not use extract.node, other R path users may use it. It has been in the package for a while

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll reach out @slucey and see if he has time to explain

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andybeet agree we should not wholly delete it, just need to make sure it's reporting what it says (at most, deprecate with warning if need be).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, so maybe we merge this (since it is just documentation) and then create an issue to look into it further and fix any issues. Even though the group may not use extract.node, other R path users may use it. It has been in the package for a while

I like that idea! I have also never used this function before for what that's worth.

@andybeet andybeet removed the request for review from sgaichas February 4, 2025 22:25
@kaydin kaydin merged commit 87950fd into docs_pre_release Feb 25, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants