search

SCBI-ForestGEO / SCBImortality

SCBI Tree Mortality
Creative Commons Attribution 4.0 International
4 stars 3 forks source link

code tests for continuous integration #2

Closed teixeirak closed 3 years ago

teixeirak commented 3 years ago

@ValentineHerr , I'm starting a running list of tests that I think would be valuable:

See this file for list

ValentineHerr commented 3 years ago

Working on this using mort20 as guinea pig for now. Looking at the set of trees that should have been censused according to 3rd main census, it looks like some Fraxinus were missed fram (65), frpe (1) and frsp (1)... they are also missing from mort19. They are on the smaller side.

I'll leave the check fail for now.

teixeirak commented 3 years ago

This test will be quite useful!

ValentineHerr commented 3 years ago

Ok I worked on the 2 first items in the list:

teixeirak commented 3 years ago

This is great; thanks!

Of course, we'll want to create a system to archive reports by year and switch over the tests to the 2021 census.

ValentineHerr commented 3 years ago

I was thinking that we don't need to archive the reports, as they should be empty once all the issues have been resolved and the census has been closed. 2020 is an exception as some stems were missed and the census was closed before that was addressed. I have not checked the previous censuses as the test is set up to look at the latest one.

teixeirak commented 3 years ago

Right, I don't see any need to archive the reports.

We don't want to apply this system to 2020, although it would be valuable to retrospectively run checks on prior censuses (lower priority).

teixeirak commented 3 years ago

@ValentineHerr , I’m getting this notice after every time I push to the repo (e.g., protocol documents). I’m just ignoring, but eventually if there’s a way to limit the tests to pushes that include data, that would be helpful.

image
ValentineHerr commented 3 years ago

Yes, I was thinking about fixing that. I need to find out how to run the test only when the raw data folder is edited. Will work on it tomorrow.

ValentineHerr commented 3 years ago

Ok @teixeirak, I think I fixed that. Now the workflow will be triggered only when a change is made in the raw_data folder (not its README.md file). I tested it and it worked for me but let me know if you still get emails when you push things in other folders.

teixeirak commented 3 years ago

Seems to be solved; thanks!

teixeirak commented 3 years ago

@ValentineHerr , I've started a list of checks to set up here (at this point just transferring the items at the top of this issue).

teixeirak commented 3 years ago

@briannakcalkins (or @jenajordan ), you can edit the file referenced above if you think of tests that should be included. I'm going to add a few more examples, and then add specific tests that I think of, but it would be great if you could add tests that seem relevant as you work with the protocol. As I mentioned yesterday, we'll set up a session with @rudeboybert to orient you to the continuous integration setup sometime next week, I think.

To edit, simply hit the pencil icon in the upper right corner, and copy the .md table formatting to add rows.

jenajordan commented 3 years ago

@teixeirak & @briannakcalkins - do checks we've discussed before (i.e. percent crown is 100% or less) go here, or have these already been incorporated)?

teixeirak commented 3 years ago

I believe that @jess-shue has constrained numerical values (e.g., percents) to possible values. I ended up adding quite a few more tests, so you can see examples of what I have in mind. These are tests that are difficult or impossible to constrain inn FastField, but important for ensuring census completion and consistency. I'll stop on this for now. One subject I haven't touched yet is the EAB census add-on.

ValentineHerr commented 3 years ago

@teixeirak, thanks for the list of tests.md file, this is very helpful. I don't think I can code those tests yet without having a template of the mort_2021.csv file (which, i understand will be a little different than 2020). But the coding should be very fast so I can work on it at the first upload or, alternatively, when Jess is done generating the form, we can fake the completion of a few trees and I can use that to test the code.

jess-shue commented 3 years ago

@teixeirak , @ValentineHerr the form is fairly close to finished at this point. One clarification: do you want to record the same extra fields for Chioanthus as you do for all of the ash trees? I can have the app show the extra section for both species; currently it only shows that section if it is an ash.

teixeirak commented 3 years ago

do you want to record the same extra fields for Chioanthus as you do for all of the ash trees?

Good question, @jess-shue . I'm actually not sure if Chioanthus would show the same symptoms of EAB as ash. Let's go ahead and include it.

teixeirak commented 3 years ago

Regarding a sample file for @ValentineHerr to work with.... Perhaps @briannakcalkins or @jenajordan could go in and generate some fake data using the form. We'll want to be sure to get at least one ash and one dead tree. Valentine could start with that, and once we get out in the field she can try the tests on real data. (She could also just code it after our first day in the field, but having a fake form would help to get the whole system running smoothly.)

Here is the link to the form: https://manage.fastfieldforms.com/WebForm/164481/Form/639377.

ValentineHerr commented 3 years ago

@teixeirak, FYI, I updated the workflow so that it looks at the FFF forms directly. Working on adding more tests now, but there is already some things to check/fix in the reports forlder

teixeirak commented 3 years ago

Thanks! This is great.

ValentineHerr commented 3 years ago

@teixeirak, in your list of test you say " DWR (dead with resprouts) not selected", where would that be entered? in the status? (can't find it in protocole nor form)

ValentineHerr commented 3 years ago

never mind! I just missed it.

ValentineHerr commented 3 years ago

@teixeirak, where do I find the key for the letters in FAD? the one I see (here) does not have wound, e.g.

ValentineHerr commented 3 years ago

@jess-shue, how do I link a tree to its photo in the excel file?

teixeirak commented 3 years ago

@ValentineHerr , here's the latest protocol version of our protocol, which @briannakcalkins is working on. And Dan's is here. Does that get what you need?

ValentineHerr commented 3 years ago

@ValentineHerr , here's the latest protocol version of our protocol, which @briannakcalkins is working on. And Dan's is here. Does that get what you need?

Ok, yes, it is in Dan's protocol, @briannakcalkins, I think you need to add "W = Wound" in the FAD table of the current protocol (and check that no other codes are missing).

teixeirak commented 3 years ago

@ValentineHerr , as discussed in lab meeting, we'll want to (1) classify reports into those that require field checking versus those that can be fixed by code (2) have the code automatically fix those that can be fixed by code, while saving a record of what has received this fix (3) also save a record of what has had to be corrected in the field

teixeirak commented 3 years ago

Oops, didn't mean to close this!

teixeirak commented 3 years ago

@ValentineHerr , my comment in the latest commit shows an example of when we need to either perform tests in a certain order or modify some tests. There may be others. The tests are all independent, correct? So I assume it's better to modify the tests than to rely on a certain order (with fixes)?

jess-shue commented 3 years ago

@jess-shue, how do I link a tree to its photo in the excel file?

@ValentineHerr This is the first time I'm using the photos option in the app and it does not present a clear connection between the stem and the images. There is a 'Comment' column that exports into the excel file with the link to the photo in the 'Photo' column. We may need to type the tag number into the 'notes' on the app for a quick fix to this issue...

ValentineHerr commented 3 years ago

Ok, now the reports should be separated into 2 types, and the CI works again.

teixeirak commented 3 years ago

Hmmm, I still don't see the 2 groups...

ValentineHerr commented 3 years ago

Make sure you refresh, I should be there:

image

teixeirak commented 3 years ago

So strange! Here's what I see (just refreshed):

image
teixeirak commented 3 years ago

oh, sorry, nevermind! I'm not in reports folder.

ValentineHerr commented 3 years ago

yeah this is a bit confusing... but the reports folder is what is important for the crew. these .R script are just part of the CI black box.

teixeirak commented 3 years ago

@ValentineHerr , would it be possible to code the rest of these tests (mainly ash census) this week? (And how much of your time do we have left?)

ValentineHerr commented 3 years ago

Sorry I had not seen the list was updated. I'll work on that now. I have ~18.5 hours left.

teixeirak commented 3 years ago

ok, thanks.

ValentineHerr commented 3 years ago

All right, I think I went through all of the items on the list, except the photo one, which I am not sure I can do, as it is unclear how the photos get paired with the data.

I added 2 things on the main repo page: one is the list of the warnings, the other is a map of the blocks with census status (done, warning pending and/or error pending).

Let me know if that works!

teixeirak commented 3 years ago

This is great; thank you! I really like that map!

What happened to all the previous errors? Are they already corrected?

teixeirak commented 3 years ago

What happened to all the previous errors? Are they already corrected?

Oh, I see that they're still there. I think that it's confusing to highlight the warnings so much more strongly than the errors. I'm going to edit the .md to fix this.