Geodetic levelling

[Aslak-Meister]

Seneste version til review primo primo januar

[krebslw]

Overordnet synes jeg det ser virkeligt godt ud, og det virker som det skal "ud-af-boksen".
Din kodestil er meget stringent og altid godt med gode docstrings :) Og synes du er lykkedes med at integrere det mere eller mindre sømløst med regnemotor-modulet.

De fleste af mine kommentarer retter sig til logikken omkring "kontrol-flowet" hvor jeg mener man kan spare en del kompleksitet. Men jeg er selv god til at finde på kringlede logiske konstruktioner i min kode, så tag ikke det hele som den absolutte sandhed :)

Dét jeg helst så rettet inden vi kan merge det, er at få dataframes helt ud af geodetic-levelling-pakken. De underliggende funktioner bør returnere de nødvendige størrelser, som det overliggende lag (regnemotor) kan pakke sammen til en dataframe og som så gemmes som excel.

[krebslw]

Kan godt se du ikke har lavet ændringer til dette, men denne del kan godt smides i sin egen funktion:

def fortolk_regneparametre(regneparametre: list[str])->dict:
    motorkwargs = {}
    for regneparameter in regneparametre:
        try:
            parameter, værdi = regneparameter.split("=")
        except ValueError:
            fire.cli.print(
                (
                    f"ADVARSEL: regneparameteren '{regneparameter} kan ikke tolkes. "
                    "Skal være på formen 'parameter=værdi'."
                ),
                bold=True,
                bg="yellow",
            )
            continue

        # konverter til float hvis der er givet en talværdi
        try:
            værdi = float(værdi)
        except ValueError:
            pass

        motorkwargs[parameter] = værdi
    return motorkwargs

[Aslak-Meister]

OK, det er implementeret.

[krebslw]

bør tolerancen kunne indstilles?

[Aslak-Meister]

Det tror jeg ikke, at en bruger har behov for.

Til gengæld kan man spare en iteration ved at slække på stopkriteriet, uden at det har nogen praktisk betydning for de konverterede højder. Jeg har blot ikke endnu beregnet, hvad stopkriteriet bør være, og denne forbedring kommer først med i næste version.

[krebslw]

self._gamle_koter er allerede en dict, så du kan erstatte det her med:

for ny_kote in self.nye_koter:
    gammel_kote= self._gamle_koter[ny_kote.punkt]
    ny_kote.nord, ny_kote.øst = gammel_kote.nord, gammel_kote.øst

Men man kunne også bare antage at self.nye_koter allerede har værdierne for nord, øst som tages fra gamle_koter. Dette vil så skulle rettes i læs_gama_outputfil, se anden kommentar.

[Aslak-Meister]

OK, jeg har valgt at ændre læs_gama_outputfil.

[krebslw]

lidt forvirrende at input-parameteren hedder det samme som funktionen. Jeg ville ændre navnet til noget andet, og så tilføje typehint:

def gamle_koter(self, punktliste: list[NivKote])->dict[str,NivKote]:
   ...

[krebslw]

lidt forvirrende at input-parameteren hedder det samme som funktionen. Jeg ville ændre navnet til noget andet, og så tilføje typehint:

def observationer(self, obsliste: list[NivObservation])->dict[str,NivObservation]:
   ...

[krebslw]

Er det nødvendigt at smide den her fejl? Vi ender kun hernede hvis fx epoch_target = 1990 og de andre to er None. Men hvis fx deformationmodel = PK_2016.tif og epoch_target=None, så fortsætter funktionen uden fejl.

Som jeg forstår det, så skal der kun laves uplift-korrektion hvis både epoch_target, deformationmodel og grid_inputfolder er givet. Hvis bare én af disse er None, så skal der ikke laves uplift-korrektion, og vi kan fortsætte til næste trin.

Suggested change

	elif epoch_target is not None:
	exit(
	"Function apply_geodetic_corrections_to_height_diffs: Wrong arguments for\n\
	parameter epoch_target and/or deformationmodel and/or grid_inputfolder."
	)

[krebslw]

Du kan forkorte alle de her udtryk via:

Suggested change

	if not tidal_system:
	self.tidal_system = None
	else:
	self.tidal_system = tidal_system
	self.tidal_system = tidal_system or None

[krebslw]

Samme som ovenfor. Alle disse kan forkortes via self.x = x or None. Måske den med grid_inputfolder istedet skal skrives:
self.grid_inputfolder = Path(grid_inputfolder) if grid_inputfolder else None

[krebslw]

Se også andre kommentarer om det samme. Generelt er det smart at tjekke for ugyldige inputs og evt. lade funktionen fejle først, sådan at man i resten af funktionen er sikker på at inputs er gyldige. Dette kan gøre det lidt lettere for uindviede at forstå hvordan funktionen fungerer, og man kan i mange tilfælde spare et indrykningsniveau, hvilket øger læsbarheden.

Fx;

if not self.height_diff_unit in ["gpu", "metric"]:
    raise ValideringFejl("Argumentet til parameteren height_diff_unit skal være enten 'metric' eller 'gpu'")

if not (
    self.tidal_system is not None
    or self.epoch_target is not None
    or self.height_diff_unit == "gpu"
):
    # Nothing to do...
    return

print("Højdeforskelle påføres geodætiske korrektioner inden udjævning")

(self.observationer, self.korrektioner_obs) = (
    apply_geodetic_corrections_to_height_diffs( ...
...

[krebslw]

Vi skal ikke have dette miljø ind i repo'et. Meningen med at have et separat miljø fire-dev-geod-lev som dette var at det kan være lettere i udviklingsfasen af skifte imellem miljøer hvor man arbejder på forskellige tilføjelser til FIRE.
Men ift. denne pull request vil det være nok at tilføje de nye afhængigheder i environment-dev.yml og environment.yml filerne.