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

Update functions florabank #61

Merged
merged 36 commits into from
Feb 19, 2025
Merged

Conversation

fredericpiesschaert
Copy link
Contributor

On 18/02/2025 database D0021_00_userFlora will be archived and replaced with a new production database D0152_00_Flora. In this pull request, the get_florabank functions are updated

@ElsLommelen
Copy link
Collaborator

Ik heb even naar de falende test gekeken, en hier is het probleem een generiek probleem omwille van checklist (waar aan gewerkt wordt).

Maar ik heb ook even de voorbeelden in de help van de functies getest, en daarvan werken er enkele niet omwille van een error bij de uitvoering van de queries. Kan jij hier even naar kijken? Het gaat over:

  • een mix van NL en wet namen invoeren in get_florabank_observations(): myspecies3 <- get_florabank_observations(db_connectie, names = c('Succisa pratensis Moench', 'Gevlekte orchis'), fixed = TRUE)
  • de voorbeelden van get_florabank_traits() (hiervan werkt er geen enkele)

Ik ga ook @hansvancalster toevoegen als reviewer. Hij had laatst een opmerking over een van de florabank-functies (een extra veld dat hij graag in de output wou hebben), dus zo kan hij als gebruiker ook feedback geven over wat voor hem belangrijk is.

@fredericpiesschaert
Copy link
Contributor Author

excuus, er stonden nog wat oude tabel- en kolomnamen in de sql, dat zou nu in orde moeten zijn

@ElsLommelen ElsLommelen force-pushed the update-functions-florabank branch from 2f23c60 to e586ace Compare February 18, 2025 15:03
Copy link
Collaborator

@ElsLommelen ElsLommelen left a comment

Choose a reason for hiding this comment

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

Nadat ik nog enkele niet meer bestaande en niet publiek toegankelijke URLs vervangen heb in het package, slagen de tests ook terug, dus voor mij is het in orde. Bedankt voor je bijdrage, @fredericpiesschaert!

@hansvancalster Heb jij nog opmerkingen? Vermits ik de laatste commit gedaan heb, denk ik dat mijn goedkeuring niet telt en dat jij ook zal moeten goedkeuren vooraleer we kunnen mergen.

@fredericpiesschaert
Copy link
Contributor Author

@ElsLommelen merci om alles na te kijken! Ik kan het blijkbaar zelf wel mergen nu maar misschien toch beter dat @hansvancalster er eerst nog eens een blik op werpt

Comment on lines 99 to 101
"SELECT DISTINCT h.Code AS hok
, CASE WHEN tmp.code IS NULL THEN h.code ELSE tmp.Code END AS ifbl_4by4
, DATEPART(year, e.BeginDatum) AS jaar
Copy link
Contributor

Choose a reason for hiding this comment

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

Om consistent te zijn zou ik hier de naamconventie van de databank overnemen en dus:

  • hok -> Hok
  • jaar -> Jaar
  • ifbl_4by4 -> Ifbl4by4 (?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ik zag ook nog ifbl_number_squares als kolomnaam wanneer ik bv get_florabank_taxon_ifbl_year( con, 1990, "4km-by-4km", "Mossen", collect = TRUE ) opvraag.

Ik heb alle functies getest en alles lijkt prima te werken 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hansvancalster nog een vraagje over dat laatste. In de beschrijving van de functie staat ook dit: in case the resolution is 4 km x 4 km, the variable ifbl_squares is a concatenation of all nested squares with observations for the taxon in the corresponding year.
Maar dat zit niet in de output van de oude functie, dus ik heb het nu ook niet toegevoegd. Ik kan dat eventueel doen maar ik zie er de meerwaarde niet van in. Gebruiken jullie dat of kan ik dat beter helemaal weglaten?

Copy link
Contributor

Choose a reason for hiding this comment

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

Dat mag inderdaad weg. Nooit gebruikt...

Copy link
Contributor

@hansvancalster hansvancalster left a comment

Choose a reason for hiding this comment

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

Ziet er goed uit 🥇 Juist nog een kleine opmerking ivm naamconventie (zie aparte comment).

Ik denk dat het ook nuttig zou zijn dat gebruikers in een vignette een paar handige commando's leren kennen voor zeer eenvoudige queries waarvoor we geen functie voorzien. Bv: de tabel met info over IFBL hokken ophalen doe je zo:

library(dplyr)
con <- inbodb::connect_inbo_dbase(database_name = "D0152_00_Flora")
hokinfo <- tbl(con, "Hok") %>% collect()
dbDisconnect(con)

En om te weten welke tabellen in de databank zitten kan je de "connections" viewer pane van RStudio raadplegen.
Zulke kleine tips dus.

Maar dat is geen onderdeel van deze PR.

@fredericpiesschaert fredericpiesschaert merged commit 64b5f20 into main Feb 19, 2025
7 checks passed
@fredericpiesschaert fredericpiesschaert deleted the update-functions-florabank branch February 19, 2025 16:13
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