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

Enhance slot CO2ppm of class swCarbon to 2-column matrix #75

Closed
dschlaep opened this issue Nov 16, 2017 · 4 comments
Closed

Enhance slot CO2ppm of class swCarbon to 2-column matrix #75

dschlaep opened this issue Nov 16, 2017 · 4 comments
Assignees

Comments

@dschlaep
Copy link
Member

The slot CO2ppm of class swCarbon is currently a vector. I believe that this should be a matrix with two columns: Year and the value. Otherwise, it is not as obvious what the code does and what any value means -- difficult to maintain.

Also this class should gain a validity method that performs the checks which are currently in rSFSW2, i.e., that all CO2 concentration values are not NA and that they are positive.

@dschlaep dschlaep added this to the Rsoilwat_co2_effects milestone Nov 16, 2017
@dschlaep dschlaep self-assigned this Nov 16, 2017
dschlaep added a commit that referenced this issue Nov 16, 2017
- slot `CO2ppm` of class `swCarbon` is now 2-column data.frame with
columns "Year" and "CO2ppm" (close #75)
- class `swCarbon` has now validity checking
- replacement functions check now validity (replacement functions for
entire carbon class slot in "swInputData"  or for CO2-concentration
data)
- added unit tests for methods related to class 'swCarbon' (close #42)
> Testing rSOILWAT2
> Carbon dioxide class: ....................
@Zachary-Kramer
Copy link
Contributor

Keep in mind that SOILWAT2 still expects a vector:

  TimeInt year;
  for (year = SW_Model.startyr + c->addtl_yr; year <= SW_Model.endyr + c->addtl_yr; year++)
  {
    c->ppm[year] = REAL(GET_SLOT(object, install("CO2ppm")))[year - 1];  // R's index is 1-based
  }

This will need to be updated, too

@dschlaep
Copy link
Member Author

done

@Zachary-Kramer
Copy link
Contributor

The code added is not functional. There is a compilation issue, but I will push the simple fix. The main issue is that there are no slots in CO2ppm (see runDataSC of any CO2 run with this code, it will show that the slot "Year" could not be accessed). I believe this is because data frames are not S4 classes, so you cannot use the GET_SLOT function on the SEXP object? I could be wrong, but I can't find any documentation online supporting the idea of passing a data.frame and accessing it with GET_SLOT.

Doesn't swWeather's approach seem correct? It uses a matrix. If swCarbon used the same approach then it could likely access the data like this: if (year == *INTEGER(GET_SLOT(VECTOR_ELT(CO2ppm, i - 1), install("Year"))))

I've tested this out a bit. Do you want to fix this or do you want me to continue looking into it?

@dschlaep
Copy link
Member Author

Yep, thanks! I working on this.

dschlaep added a commit that referenced this issue Nov 18, 2017
- all unit tests pass except 3 in scope "rSOILWAT2 annual aggregation"
    --> close #42, close #74, close #75, close #76, close #77
- the unit test errors are because the values of the output slot
"CO2EFFECTS" are not calculated with aggregation function that is
specified --> see new issue #78
N1ckP3rsl3y pushed a commit that referenced this issue Jun 11, 2022
- addressing #74 (Include detailed user manual in doxygen documentation)
- close #75 (Include detailed installation instructions in doxygen documentation)

- subpages for inputs and outputs are currently only place holders
N1ckP3rsl3y pushed a commit that referenced this issue Jun 11, 2022
This version addressed documentation and unit testing:

User visible changes:
- Create and open documentation and user manual with `make doc doc_open`

Details:
- Documentation:
  - 'User manual' for SOILWAT2 provided as part of doxygen documentation (close #217, close #74)
  - Detailed installation instructions included in doxygen documentation (close #75)
  - Code contributor manual as part of doxygen documentation (close 86)
  - Doxygen warnings fixed (close #266)
  - Input units of biomass documented (close #265)

- Unit tests:
  - Contributed to the documentation and unit testing for all SOILWAT2 functions (see #216, see #219, see #76; close #73)
  - Unit tests for functions in `SW_Flow_lib.c` (close #117; contributed to #19)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants