Fims vector #517

ChristineStawitz-NOAA · 2023-11-07T21:23:22Z

What is the feature?

Adds a fims_vector class that does conversions between std:: and tmbutils:: vector types

How have you implemented the solution?

add new fims_vector.hpp file
update fleet.hpp and population.hpp files to use fims::Vector type
resolve merge conflicts regarding namespace and logging updates

Does the PR impact any other area of the project?

How to test this change

Developer pre-PR checklist

I relied on GitHub actions to 🧪 things for me while I sat on the 🛋️.

github-actions · 2023-11-07T21:23:41Z

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).

ChristineStawitz-NOAA · 2023-11-07T21:24:54Z

note: we still need to remove some calls to ParameterVector and delete ModelTraits, but given how many merge conflicts we are experiencing think those should be fixed in a new branch to keep this change small.

codecov · 2023-11-07T21:28:15Z

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (8fc62f5) 74.92% compared to head (b945e85) 75.40%.

Files	Patch %	Lines
inst/include/common/fims_vector.hpp	95.12%	0 Missing and 2 partials ⚠️
...erface/rcpp/rcpp_objects/rcpp_tmb_distribution.hpp	50.00%	2 Missing ⚠️
...include/interface/rcpp/rcpp_objects/rcpp_fleet.hpp	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #517      +/-   ##
==========================================
+ Coverage   74.92%   75.40%   +0.48%     
==========================================
  Files          38       39       +1     
  Lines        2042     2090      +48     
  Branches      136      140       +4     
==========================================
+ Hits         1530     1576      +46     
  Misses        471      471              
- Partials       41       43       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ChristineStawitz-NOAA · 2023-11-07T22:33:42Z

@msupernaw doxygen is throwing this error: error: Member value_type (typedef) of class fims::Vector is not documented. (warning treated as error, aborting now)

Can you please add documentation on value_type?

ChristineStawitz-NOAA · 2023-12-04T23:28:42Z

@msupernaw can you put the description of "friend" in the documentation?

Added description to friend operator.

msupernaw · 2023-12-04T23:58:40Z

@christine Stawitz - NOAA Federal ***@***.***> done.

…

On Mon, Dec 4, 2023 at 6:28 PM Christine Stawitz - NOAA < ***@***.***> wrote: @msupernaw <https://github.com/msupernaw> can you put the description of "friend" in the documentation? — Reply to this email directly, view it on GitHub <#517 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFUSEE4E6TM73O7VVKKEELYHZMDPAVCNFSM6AAAAAA7B3HKHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZZG4YTONRZG4> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Matthew Supernaw *Scientific Software Developer* *National Oceanic and Atmospheric Administration* *Office Of Science and Technology* *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

recruit devs check

inst/include/common/fims_vector.hpp

msupernaw · 2023-12-07T19:14:23Z

This <https://github.com/NOAA-FIMS/FIMS/blob/fims_vector/tests/integration/integration_class.hpp#L565> line seems incorrect. Any idea why the index for rdev is i +1?

…

On Thu, Dec 7, 2023 at 1:00 PM Andrea-Havron-NOAA ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In inst/include/common/fims_vector.hpp <#517 (comment)>: > + + /** + * @brief Swaps the contents. + */ + inline void swap( Vector& other ){ + this->vec_m.swap(other.vec_m); + } + + // end std::vector functions + + /** + * Conversion operators + */ + + /** + * @brief Converts fims::Vector to std::vector<Type> edit to fims::Vector<Type> — Reply to this email directly, view it on GitHub <#517 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFUSEFKS5TLPTCSPEU2S2LYIH72FAVCNFSM6AAAAAA7B3HKHWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZQG43DANZWGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Matthew Supernaw *Scientific Software Developer* *National Oceanic and Atmospheric Administration* *Office Of Science and Technology* *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

msupernaw · 2023-12-07T21:00:02Z

Yeah, This <https://github.com/NOAA-FIMS/FIMS/blob/fims_vector/tests/integration/integration_class.hpp#L565> line is definitely overreaching the rdev array causing the random value in the last index of rec->log_recruit_devs. rdev has a length of nyears just like rec->log_recruit_devs. On Thu, Dec 7, 2023 at 2:14 PM Matthew Supernaw - NOAA Federal < ***@***.***> wrote:

…

This <https://github.com/NOAA-FIMS/FIMS/blob/fims_vector/tests/integration/integration_class.hpp#L565> line seems incorrect. Any idea why the index for rdev is i +1? On Thu, Dec 7, 2023 at 1:00 PM Andrea-Havron-NOAA < ***@***.***> wrote: > ***@***.**** commented on this pull request. > ------------------------------ > > In inst/include/common/fims_vector.hpp > <#517 (comment)>: > > > + > + /** > + * @brief Swaps the contents. > + */ > + inline void swap( Vector& other ){ > + this->vec_m.swap(other.vec_m); > + } > + > + // end std::vector functions > + > + /** > + * Conversion operators > + */ > + > + /** > + * @brief Converts fims::Vector to std::vector<Type> > > edit to fims::Vector<Type> > > — > Reply to this email directly, view it on GitHub > <#517 (review)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABFUSEFKS5TLPTCSPEU2S2LYIH72FAVCNFSM6AAAAAA7B3HKHWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONZQG43DANZWGA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > -- Matthew Supernaw *Scientific Software Developer* *National Oceanic and Atmospheric Administration* *Office Of Science and Technology* *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

-- Matthew Supernaw *Scientific Software Developer* *National Oceanic and Atmospheric Administration* *Office Of Science and Technology* *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

ChristineStawitz-NOAA · 2023-12-07T21:26:56Z

@msupernaw - I think this is the same issue I had to fix in the R integration test but then I never went back and made the same fix in the C++ integration test. Is it fixed now in the C++ integration test, or should I fix it?

msupernaw · 2023-12-07T23:29:05Z

@christine Stawitz - NOAA Federal ***@***.***> I just took care of it! All good!

…

On Thu, Dec 7, 2023 at 4:27 PM Christine Stawitz - NOAA < ***@***.***> wrote: @msupernaw <https://github.com/msupernaw> - I think this is the same issue I had to fix in the R integration test but then I never went back and made the same fix in the C++ integration test. Is it fixed now in the C++ integration test, or should I fix it? — Reply to this email directly, view it on GitHub <#517 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABFUSEESQNNP5OBONH7V7XTYIIYCZAVCNFSM6AAAAAA7B3HKHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBWGEZTOMJUGA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Matthew Supernaw *Scientific Software Developer* *National Oceanic and Atmospheric Administration* *Office Of Science and Technology* *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

msupernaw · 2023-12-07T23:41:53Z

@christine Stawitz - NOAA Federal ***@***.***> Now all platforms are failing for test 42, but the rec devs make sense! I think this needs review. On Thu, Dec 7, 2023 at 6:28 PM Matthew Supernaw - NOAA Federal < ***@***.***> wrote:

…

@christine Stawitz - NOAA Federal ***@***.***> I just took care of it! All good! On Thu, Dec 7, 2023 at 4:27 PM Christine Stawitz - NOAA < ***@***.***> wrote: > @msupernaw <https://github.com/msupernaw> - I think this is the same > issue I had to fix in the R integration test but then I never went back and > made the same fix in the C++ integration test. Is it fixed now in the C++ > integration test, or should I fix it? > > — > Reply to this email directly, view it on GitHub > <#517 (comment)>, or > unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABFUSEESQNNP5OBONH7V7XTYIIYCZAVCNFSM6AAAAAA7B3HKHWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNBWGEZTOMJUGA> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> > -- Matthew Supernaw *Scientific Software Developer* *National Oceanic and Atmospheric Administration* *Office Of Science and Technology* *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

-- Matthew Supernaw *Scientific Software Developer* *National Oceanic and Atmospheric Administration* *Office Of Science and Technology* *NOAA Fisheries | *U.S. Department of Commerce Phone 248 - 396 - 7797

ChristineStawitz-NOAA · 2023-12-11T19:12:52Z

@Andrea-Havron-NOAA and I are looking at this and other than the comment edits, which I can fix, there is an issue with code_cov that begin() and end() are not tested when they are const iterators. It doesn't have a problem with begin() or end() for non-const iterators. Can we either add a test for const iterators in the integrated or unit test, or remove these methods?

msupernaw · 2023-12-11T20:18:29Z

Seems easy enough to add a unit test, although I curious why it’s only an issue with constant iterators!Sent from my iPhoneOn Dec 11, 2023, at 2:13 PM, Christine Stawitz - NOAA ***@***.***> wrote: @Andrea-Havron-NOAA and I are looking at this and other than the comment edits, which I can fix, there is an issue with code_cov that begin() and end() are not tested when they are const iterators. It doesn't have a problem with begin() or end() for non-const iterators. Can we either add a test for const iterators in the integrated or unit test, or remove these methods? —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

ChristineStawitz-NOAA requested review from a team and MOshima-PIFSC and removed request for a team November 7, 2023 21:23

ChristineStawitz-NOAA requested a review from Andrea-Havron-NOAA November 7, 2023 21:23

msupernaw and others added 19 commits November 7, 2023 13:24

added fims vector

314a4a8

updates

fddb9e9

Update fims_vector.hpp

dd50459

fully wrapped std::vector

37c40d8

Update fims_vector.hpp

26533e5

Update fims_vector.hpp

e50c35e

added fims::Vector to population.

3f6160e

use new fims::vector for fleet agecomp nll

940c8a3

add constructor for fims_vector

ec0a10c

Update fims_vector.hpp

c0d56b1

Update fims_vector.hpp

da128c9

Update fims_vector.hpp

21b7abc

Update fims_vector.hpp

236bfaf

Update fims_vector.hpp

3e2506a

Update fims_vector.hpp

37a71f6

Update fims_vector.hpp

62075c7

Update fims_vector.hpp

69ab198

Update fims_vector.hpp

6d4b460

resolve merge conflict errors

fa2648c

ChristineStawitz-NOAA force-pushed the fims_vector branch from 2372ec7 to fa2648c Compare November 7, 2023 21:24

ChristineStawitz-NOAA assigned msupernaw Nov 7, 2023

remove parametervector calls in fleet and pop

bb260c5

Update fims_vector.hpp

7c3320f

Added description to friend operator.

msupernaw added 3 commits December 5, 2023 11:50

Update integration_class.hpp

b878114

recruit devs check

Update fims_vector.hpp

6dc3985

Update rcpp_tmb_distribution.hpp

e33f89a

Andrea-Havron-NOAA reviewed Dec 7, 2023

View reviewed changes

inst/include/common/fims_vector.hpp Outdated Show resolved Hide resolved

msupernaw added 7 commits December 7, 2023 13:21

Update fims_vector.hpp

7146751

Update fims_vector.hpp

9229cfc

Update integration_class.hpp

1e45e97

Update integration_class.hpp

aa2dcb0

Update integration_class.hpp

8073c39

Update integration_class.hpp

f65e43e

Update integration_class.hpp

468959b

Update integration_class.hpp

48bd94e

Update integration_class.hpp

06fc54a

ChristineStawitz-NOAA added 4 commits December 11, 2023 16:06

cleanup of commented code and dox

664eed7

make log_obs_error a vector

19eb97c

make log_obs_error a vector (#532)

69dda10

remove begin() and end() for const iterator

b945e85

Andrea-Havron-NOAA approved these changes Dec 13, 2023

View reviewed changes

ChristineStawitz-NOAA merged commit b58aaee into main Dec 13, 2023
13 checks passed

ChristineStawitz-NOAA deleted the fims_vector branch December 13, 2023 17:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fims vector #517

Fims vector #517

ChristineStawitz-NOAA commented Nov 7, 2023

github-actions bot commented Nov 7, 2023

ChristineStawitz-NOAA commented Nov 7, 2023

codecov bot commented Nov 7, 2023 •

edited

Loading

ChristineStawitz-NOAA commented Nov 7, 2023

ChristineStawitz-NOAA commented Dec 4, 2023

msupernaw commented Dec 4, 2023 via email

msupernaw commented Dec 7, 2023 via email

msupernaw commented Dec 7, 2023 via email

ChristineStawitz-NOAA commented Dec 7, 2023

msupernaw commented Dec 7, 2023 via email

msupernaw commented Dec 7, 2023 via email

ChristineStawitz-NOAA commented Dec 11, 2023

msupernaw commented Dec 11, 2023 via email

Fims vector #517

Fims vector #517

Conversation

ChristineStawitz-NOAA commented Nov 7, 2023

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

github-actions bot commented Nov 7, 2023

Instructions for code reviewer

Checklist

ChristineStawitz-NOAA commented Nov 7, 2023

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

ChristineStawitz-NOAA commented Nov 7, 2023

ChristineStawitz-NOAA commented Dec 4, 2023

msupernaw commented Dec 4, 2023 via email

msupernaw commented Dec 7, 2023 via email

msupernaw commented Dec 7, 2023 via email

ChristineStawitz-NOAA commented Dec 7, 2023

msupernaw commented Dec 7, 2023 via email

msupernaw commented Dec 7, 2023 via email

ChristineStawitz-NOAA commented Dec 11, 2023

msupernaw commented Dec 11, 2023 via email

codecov bot commented Nov 7, 2023 •

edited

Loading