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

added to_kingdom function #32

Merged
merged 7 commits into from
Sep 3, 2024
Merged

added to_kingdom function #32

merged 7 commits into from
Sep 3, 2024

Conversation

tommervermaas
Copy link
Contributor

new function to export data for the seismic interpretation software Kingdom

Copy link
Collaborator

@Erik-Geo Erik-Geo left a comment

Choose a reason for hiding this comment

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

Alles staat op juiste plek en werkt. mooi!

Paar comments vooral op testing en typing.

Gebruik je ook black formatting? Die zou namelijk spaties moeten zetten in functie definities (dus bijvoorbeeld vw = 1500.0 ipv vw=1500.0).

geost/base.py Outdated
@@ -1,5 +1,5 @@
import pickle
from pathlib import WindowsPath
from pathlib import Path, WindowsPath
Copy link
Collaborator

Choose a reason for hiding this comment

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

Path hoeft niet geimporteerd te worden, zie hieronder

geost/base.py Outdated

tdchart.drop("surface", axis=1, inplace=True)
tdchart.sort_values(by=["id", "MD"], inplace=True)
outfile = Path(outfile.parent, f"{outfile.stem}_TDCHART{outfile.suffix}")
Copy link
Collaborator

Choose a reason for hiding this comment

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

outfile.parent.joinpath(f"{outfile.stem}_TDCHART{outfile.suffix}")

-> Dan hoeft Path ook niet geimporteerd te worden

geost/base.py Outdated
vs : float
sound velocity in sediment in m/s, default is 1600 m/s
"""
self.data.to_kingdom(outfile, vw, vs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Deze wordt gemist in tests. -> ook nog test vanuit collection schrijven

@@ -465,6 +465,16 @@ def test_to_qgis3d(self, borehole_collection):
assert outfile.is_file()
outfile.unlink()

@pytest.mark.unittest
def test_to_kingdom(self, borehole_data):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Borehole_data is layeredData object, vandaar dat (zie vorige comment) het aanroepen van de collection method niet getest wordt.

Gebruik in plaats daarvan de borehole_collection fixture

@@ -270,22 +270,32 @@ def test_to_qgis3d(self, borehole_data):
assert outfile.is_file()
outfile.unlink()

@pytest.mark.unittest
def test_to_kingdom(self, borehole_data):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Er worden wat situaties gemist door de if/else statements in de export functie. Zou je die ook nog kunnen testen?

geost/base.py Outdated

Parameters
----------
outfile : Union[str, WindowsPath]
Copy link
Collaborator

Choose a reason for hiding this comment

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

str | WindowsPath

Gebruyik van Union typing is sinds Python 3.11 niet meer nodig

geost/base.py Outdated
@@ -1110,6 +1110,65 @@ def to_qgis3d(
qgis3d = self._create_geodataframe_3d(relative_to_vertical_reference)
qgis3d.to_file(outfile, driver="GPKG", **kwargs)

def to_kingdom(self, outfile, vw=1500.0, vs=1600.0):
Copy link
Collaborator

Choose a reason for hiding this comment

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

typing staat ook in de functiedefinitie:

def to_kingdom(self, outfile: str | WindowsPath, vw: float = 1500.0, vs: float = 1600.0

geost/base.py Outdated
@@ -2041,6 +2100,22 @@ def to_qgis3d(
"""
self.data.to_qgis3d(outfile, relative_to_vertical_reference, **kwargs)

def to_kingdom(self, outfile, vw=1500.0, vs=1600.0):
Copy link
Collaborator

Choose a reason for hiding this comment

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

typing staat ook in de functiedefinitie:

def to_kingdom(self, outfile: str | WindowsPath, vw: float = 1500.0, vs: float = 1600.0

Copy link
Collaborator

@Erik-Geo Erik-Geo left a comment

Choose a reason for hiding this comment

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

No een paar vragen de functielogica...

geost/base.py Outdated
"""
# total depth of borehole in m
if "Total depth" in self.df.columns:
self["Total depth"] = self["surface"] - self["end"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

De dataframe wordt nu blijvend aangepast voor het hele object, maar dit kan beter alleen binnen de exportfunctie gehouden worden.

geost/base.py Outdated
self.df.insert(7, "Total depth", (self["surface"] - self["end"]))
# start depth of interval in m
if "Start depth" in self.df.columns:
self["Start depth"] = self["top"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Als dat het geval is dan hoeft er toch niks te gebeuren -> pass?

@Erik-Geo
Copy link
Collaborator

Erik-Geo commented Sep 2, 2024

Nog even over nagedacht en zou het zo aanpakken:

  • Ik zou een nieuwe tijdelijke dataframe maken die alleen voor de export gebruikt wordt.
  • Altijd de verplichte kolommen (Start_depth, Total_depth, End_depth) berekenen vanuit LayeredData.df (mogen die namen van Kingdom met kleine letters beginnen?). Dit berekenen is allemaal vectorized en dus snel genoeg.
  • Nieuw dataframe vullen met alle verplichte kolommen (neem aan ook x, y, naast de hierboven berekende laaggrenzen) en verder aanvullen met alle datakolommen die er zijn.
  • Deze dataframe to_csv

Dat lost een aantal dingen meteen op. Zo heb je geen if/else statements meer die extra testsituaties vereisen. Wel zou ik nog testen of je nieuw gemaakte kolommen voldoen aan de verwachtingen (dus Start_depth, Total_depth, End_depth). Ook blijft LayeredData.df onaangepast.

@Erik-Geo Erik-Geo merged commit 5cf2d99 into main Sep 3, 2024
7 checks passed
@Erik-Geo Erik-Geo deleted the feature/kingdom_export branch October 16, 2024 08:53
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.

2 participants