Reg al #100
Reg al #100
Conversation
foutje bij berekening percentage subplots met verjonging
- uitleg bij CCC - kortere naamgeving cumm_herb_coverage
Merge branch 'reg_AL' of https://github.com/inbo/forrescalc into reg_AL # Conflicts: # R/load_plotinfo.R
- join plotinfo without "year": dat komt in de resp. plot-datasets reeds voor. Maar het leek me toch noodzakelijk dat "year" ook in plotinfo zat. (kan niet bij "join by" omdat de year in plotinfo, van dendro komt, en bij bv. reg of veg anders zou kunnen zijn)
- join ook by "period", want nu ook info over data_processed per periode in plotinfo
…g originele naam gegeven (dan geen verwarring)
… correct. (één plot (11000) waar ook bij periode 1 oldid's ingevuld staan (link met 1986))
…lijkheidsoplossing oms problemen bij compose_stem_data te voorkomen (kan properder door bij functie compose_stem_data iufro-variabelen weg te filteren, maar dan wel met de vwde dat ze ingeladen zijn, het zijn immers optionele variabelen. En ik weet niet hoe ik "select(-contains("iufro"))" moet gebruiken in een package))
Co-authored-by: ElsLommelen <els.lommelen@inbo.be>
Co-authored-by: ElsLommelen <els.lommelen@inbo.be>
Co-authored-by: ElsLommelen <els.lommelen@inbo.be>
|
Els, ik denk dat ik al je opmerkingen behandeld heb (reeds in april), en issues als resolved aangeduid. |
|
@ElsLommelen : heel raar, ik dacht dat ik vorige week hier nog een extra comment toegevoegd had, maar ik zie die nu niet meer staan? Daarnaast bedacht ik ook dat het beter was om |
Bekijk sowieso eerst nog eens wat het laatste is wat we hierover afgesproken hebben, want door die benamingen en logica weer eens aan te passen, gaan we weer overal nieuwe problemen creëren, vrees ik. Hieruit blijkt alvast dat we recent nog afgesproken hebben om die te behouden (en dat ze niet overal dezelfde waarde hebben), dus misschien ga je best eerst eens grondig nagaan of dit verwijderen wel kan en of die nergens gebruikt wordt in andere functies? In verband met het foutje: als het een klein foutje is en je goed oplet om geen typfouten te maken, kan je het altijd op github zelf toevoegen. Maar je mag uiteraard ook wachten tot het in orde is. (Je comment van enkele dagen geleden zie ik hier wel, mss je pagina eens refreshen?) |
…nd "nr_seedlings_ha") based on exact numbers. This for all 5 "calculate _regeneration"-scripts.
…_of_regeneration_ha (and nr_established_ha/nr_seedlings_ha)
Ik weet niet goed hoe dit komt. Dit is mijn manier om dit foutje te corrigeren (mag ook anders)
calculate_reg_core_area_species: toevoegen van extra code om te corrigeren voor NA van established of seedlings wanneer er resp. wel seedlings of established reg is (dus dan moet dat 0 zijn. Mogelijks kan jij dat compacter of properder. Maar deze code doet wel wat het moet doen.
|
@ElsLommelen : ik ben rond met mijn aanpassingen. |
There was a problem hiding this comment.
Ik heb de aanpassingen t.o.v. vorige versies doorgenomen, en op basis daarvan heb ik alvast de volgende opmerkingen genoteerd (vooral details, geen fundamentele opmerkingen).
Ik zal morgen ook nog even de scripts zelf testen (a.d.h.v. voorbeelden en main), om zeker te zijn dat aanpassingen in de naamgeving geen problemen veroorzaken.
| by_core_area_species <- data_herblayer %>% | ||
| group_by(.data$plot_id, .data$period) %>% | ||
| mutate( | ||
| n_subplots = n_distinct(.data$subplot_id) |
There was a problem hiding this comment.
@ElsLommelen : hier nog een foutje ontdekt: n_subplots zou moeten bepaald worden obv data_vegetation, want in data_herblayer zitten enkel de subplots waar minimum één soort waargenomen werd. In data_vegetation zitten alle subplots.
(Bij regeneration wordt n_subplots bepaald obv de regeneration layer, wat wel correct is)
There was a problem hiding this comment.
idem dito in calc_veg_core_area_height_species
There was a problem hiding this comment.
Euh, dit is een vreemde situatie die je best nog even bekijkt, @leymanan :
Blijkbaar heb je in deze functie het argument data_vegetation toegevoegd zonder het te gebruiken (zie hier), en ook zonder de documentatie hieraan aan te passen. Maar dit argument lijkt me niet nodig te zijn? Mag dit terug weg? (Zie eerst hieronder.)
@ElsLommelen : hier nog een foutje ontdekt:
n_subplotszou moeten bepaald worden obvdata_vegetation, want indata_herblayerzitten enkel de subplots waar minimum één soort waargenomen werd. Indata_vegetationzitten alle subplots.
(Bij regeneration wordtn_subplotsbepaald obv de regeneration layer, wat wel correct is)
De subplot_id die opgehaald wordt in load_data_herblayer(), is afkomstig uit tabel Vegetation uit de databank (zie hier), dus ik ga ervan uit dat dit wel in orde is?
idem dito in
calc_veg_core_area_height_species
Functie calc_veg_core_area_height_species() zie ik nergens staan, kan het zijn dat die nooit toegevoegd is? Moet die toegevoegd worden?
There was a problem hiding this comment.
in deze commit zie ik anders wel gebruik van data_vegetation?
ik denk nog steeds dat dit nodig is.
De
subplot_iddie opgehaald wordt inload_data_herblayer(), is afkomstig uit tabel Vegetation uit de databank (zie hier), dus ik ga ervan uit dat dit wel in orde is?
daar wordt een inner-join gemaakt tss Herblayer en Vegetation: dus als er een bepaald record niet aanwezig is in Herblayer (wegens geen enkele soort aanwezig), dan wordt die subplot_id niet opgepikt ...
Functie
calc_veg_core_area_height_species()zie ik nergens staan, kan het zijn dat die nooit toegevoegd is? Moet die toegevoegd worden?
Height wordt enkel bij regeneration gebruikt (calculate_regeneration_core_area_height_species.R bestaat)
calc_veg_core_area_height_species() niet nodig ;-)
There was a problem hiding this comment.
in deze commit zie ik anders wel gebruik van data_vegetation?
Dat is een commit van meer dan 2 jaar geleden, en intussen is de code blijkbaar veranderd en is het inladen van data_vegetation overbodig geworden. Ik heb even gezocht waar die data_vegetation gewist is, maar ik kan het niet terugvinden. Ik vermoed dat we dat we op een bepaald moment beslist hebben om die subplot_id in load_data_herblayer() op te halen om gebruikers niet te verplichten om voor calculate_vegetation_core_area_species() behalve load_data_herblayer() ook load_data_vegetation() te moeten opladen enkel om dat ene item subplot_id op te laden. Maar blijkbaar zijn we daarbij vergeten om de join in load_data_herblayer() aan te passen.
daar wordt een inner-join gemaakt tss Herblayer en Vegetation: dus als er een bepaald record niet aanwezig is in Herblayer (wegens geen enkele soort aanwezig), dan wordt die
subplot_idniet opgepikt ...
Zal ik er daar een left join van maken? Dus Plots inner join Vegetation LEFT JOIN Herblayer. (Vermits het access is, zal ik ook nog met de haakjes moeten spelen enz., maar ik neem aan dat je wel weet wat ik bedoel.) Dit lijkt me een veel minder ingrijpende aanpassing als het functioneel toevoegen van data_vegetation in calculate_vegetation_core_area_species().
There was a problem hiding this comment.
ja, dat is inderdaad betere oplossing (left join bij load data herblayer)
Maar indien mogelijk misschien wel survey_veg = TRUE ergens meenemen?
There was a problem hiding this comment.
Ok, opgelost in commit cf90684 (left join in Access ziet er wel iets complexer uit dan gewone SQL omdat hier de mix van inner join en left join in Access niet kan zonder met een subquery te werken).
Die selectie survey_veg = TRUE gebeurt blijkbaar hier in load_data_herblayer(), dus dat zou ook in orde moeten zijn.
Co-authored-by: Els Lommelen <els.lommelen@inbo.be>
Co-authored-by: Els Lommelen <els.lommelen@inbo.be>
Co-authored-by: Els Lommelen <els.lommelen@inbo.be>
…fr voorstel Els L.
| @@ -19,7 +27,8 @@ | |||
| #' | |||
| #' @export | |||
| #' | |||
| #' @importFrom dplyr %>% group_by left_join mutate n_distinct select summarise ungroup | |||
| #' @importFrom dplyr %>% group_by left_join mutate n_distinct select summarise | |||
| #' ungroup | |||
| #' @importFrom rlang .data | |||
| #' | |||
| calculate_vegetation_plot <- function(data_vegetation, data_herblayer) { | |||
There was a problem hiding this comment.
in scriptregel hierna wordt vertrokken van herblayer, maar dat zou van data_vegetation moeten zijn, anders mis je de plots zonder soorten (momenteel weliswaar maar één plot: plot_id 1176, period 1).
Of verderop full_join met data_vegetation ipv left_join
There was a problem hiding this comment.
Ik vermoed dat dit beter in de query in load_data_herblayer() opgelost wordt? Hier worden sowieso de id's van tabel vegetatie mee binnengehaald, en ook de info over het jaar van het onderzoek, dus ik veronderstel dat ik best daar de query aanpas zodat alle plots meegenomen worden, ook die zonder soorten en records in herblayer? Of zie ik iets over het hoofd waardoor die optie toch niet wenselijk is?
There was a problem hiding this comment.
nee, is ok, maar kan je hier ook zorgen dat enkel deze waar een opname gebeurd is, meegenomen worden (survey_veg = TRUE), want voor start veldseizoen worden in Fieldmap automatisch alle vegetatie records aangemaakt (om gis-component te hebben)
There was a problem hiding this comment.
Ik heb de functie getest en de hierboven vermelde plot (plot_id 1176) wordt inderdaad binnengehaald en heeft voor periode 1 maar 1 record bij NA voor de soorten. Als ik de hele dataset bekijk (CP en CA over alle periodes heen), zijn er 122 records met NA, vnl. afkomstig van 3 CA-plots uit de eerste periode: Kersselaerspleyn, Hannecart en Rodebos KV2. Maar het kan dat ik met een hopeloos verouderde versie van de databank aan 't werken ben.
Misschien dat jij bij gelegenheid best ook nog even test of het in orde is? (Moet niet direct nu, maak gerust een lijstje om eens te doen als het uitkomt, ik ga hier toch nog wel een tijdje mee bezig zijn. ;-) )
| @@ -40,7 +49,8 @@ calculate_vegetation_plot <- function(data_vegetation, data_herblayer) { | |||
| left_join( | |||
There was a problem hiding this comment.
ev. full_join ipv left_join? (zie ook hoger, vorige comment)
There was a problem hiding this comment.
Wat doe ik in dat geval met number_of_species en cumm_herb_coverage_class_average_perc? Als ze geen overeenkomstige waarden hebben in data_vegetation, gaan ze NA zijn. Is dat ok, of moet dat dan 0 worden?
Sowieso, gezien het feit dat load_data_herblayer() ook alle plots meeneemt, zou dit niet kunnen voorkomen, tenzij de gebruiker in load_data_vegetation() niet dezelfde dataset opgevraagd heeft (bv. ander plottype of forest_reserve). Dus ik zou ook een anti_join kunnen doen als foutencheck, of is dat niet nodig?
Enfin, laat maar weten of er nog iets moet gebeuren met je opmerking, daarna kunnen we dit hoofdstuk (opmerkingen reg_AL) stilaan afsluiten.
There was a problem hiding this comment.
reden voor full_join is achterhaald, opgevangen in load_data_herblayer()
Nu bevat data_herblayer immers ook al de "lege" plots, zonder kruidlaag (species = NA)
Co-authored-by: Anja Leyman <anja.leyman@inbo.be>
|
@leymanan Ik ben - denk ik - ooit eens gestart met een branch bug-fixes vanuit deze branch met het idee om enkele fixes voor te stellen en die dan in deze branch te mergen, maar blijkbaar ben ik er nooit toe gekomen om deze af te werken en een PR te maken. (In elk geval is er een branch bug-fixes die start vanaf de laatste commit van deze branch.) Zou het een idee zijn om gewoon met die bug-fixes verder te gaan, daar alle issues in op te lossen, en dan die uiteindelijk rechtstreeks naar de master te mergen na review door u? |
Had eerst niet door wat je bedoelde met "Zijn er hierboven nog zaken uit de commentaren die ingewerkt moeten worden? Want dan is dat misschien niet de beste optie."? Of wat bedoel je met "moeten ingewerkt worden"? Ik heb die allemaal ooit beantwoord. |
bug-fixes is vertrokken van reg_AL na de commits van de comments, dus dat is niet het probleem. Probleem is vooral: zijn alle andere commentaren ingewerkt? Je hebt inderdaad overal wel op gereageerd, maar het is niet altijd duidelijk of er ook actie ondernomen is. Enfin, ik ben intussen bezig om dat stap voor stap te checken, en eventuele nodige aanpassingen rechtstreeks in bug-fixes te doen. En waar onduidelijkheid is over de status, voeg ik ook een nota toe, evt. gewoon om aan te geven dat het opgelost is. En ik zal ze ook resolven. |
|
@leymanan Zou jij nog eens even willen kijken of deze PR dicht mag? Situatie is dat testdatabase (of ene voorganger) vertrokken is van deze branch, dus alle commits zitten in de huidige versie en de PR zelf moet dus niet gemerged worden. Maar hierin staan (zeker op het einde na de commits) nog een aantal opmerkingen waarvan voor mij niet duidelijk is of hier wel gevolg aan gegeven is. Soms weet ik niet goed wat je bedoelt of waarom je iets wil, dus voor mij is het heel moeilijk inschatten of er nog iets relevants tussen zit. Zo stel je bv. in je laatste opmerking op het script voor om een left_join() te vervangen door een full_join(), maar ik snap niet waarom, en op mijn vragen hierover is nooit een reactie gekomen. En zo zijn er nog enkele opmerkingen hierboven. Kan jij eens nakijken of er nog iets tussen zit waar best toch nog gevolg aan gegeven kan worden? (Je laatste review is goed hoor.) Als het voor u allemaal in orde is, mag deze PR voor mij gesloten. |
No description provided.