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

Fix/thicknesses_ #112

Draft
wants to merge 36 commits into
base: master
Choose a base branch
from
Draft

Fix/thicknesses_ #112

wants to merge 36 commits into from

Conversation

AngRodrigues
Copy link
Member

@AngRodrigues AngRodrigues commented Jul 9, 2024

Summary of major changes:

  • Thickness outputs from all thickness calculators are now merged
  • LPF version pinned to 1.4
  • Added folder with datasets in map2loop/_datasets/geodata_files/hamersley. The idea behind this is to have a data loading function, e.g., at the moment load_hamersley_geology() returns a gdf with the geology; structure and dtm are available, and others will be added later on. This is quite useful for tests, but should also be useful for when we implement [Feature Request] - create map2loop project from geopandas array not file paths #74. The only thing about this is the location, which implies a long import: from map2loop._datasets.geodata_files.hamersley import load_hamersley_geology. Happy to change the file location - does anyone has a suggestion?
  • Minimum fault length is now a parameter of the config file, and it actually removes the faults with length under defined value.
  • Removed the project.set_minimum_fault_length method to avoid users setting the parameter twice.
  • added all_basal_contacts object which are abnormal+basal contacts , and basal_contacts holds only the basal. Reconstruction of sampled_basal_contacts is based on basal_contacts
  • changed the thickness_calculator_alpha to be coherent with the logic of the other thickness calculators
  • added tests for some of the functions in mapdata.py
  • added tests for thickness_compute in project.py
  • rewrote tests for all thickness calculators independently (now without having to run the project)

summary of other minor modifications:

  • tmpfile is now used to create the localGdalfile
  • changed to f"" strings where possible
  • change from os.path.join to pathlib.Path where possible
  • added geopandas to csv converter in mapdata (it was on a #TODO)
  • added check and error message to make sure that all units in user_defined_stratigraphic_column are in the geology file (otherwise project creation fails; at least now user has a warning).
  • renamed distance to stratigraphic_distance in basal_contacts creation
  • removed set_ and get_thickness_calculator
  • rectified minor typos in the thickness calculators
  • added a Linalgerror catch/pass for the InterpolatedStructure

Fixes #86
Fixes #110
Fixes #111
Fixes #12

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Test improvement

How Has This Been Tested?

The branch is fix/thicknesses_AR_2.

Checklist:

  • This branch is up-to-date with master
  • All gh-action checks are passing
  • I have performed a self-review of my own code
  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have added tests that prove my fix is effective or that my feature works
  • My tests run with pytest from the map2loop folder
  • New and existing tests pass locally with my changes

@AngRodrigues AngRodrigues marked this pull request as ready for review July 9, 2024 00:35
@AngRodrigues AngRodrigues changed the title Fix/thicknesses ar 2 Fix/thicknesses_ Jul 9, 2024
@@ -30,7 +30,8 @@ def __init__(self):
"""
The initialiser for the deformation history. All attributes are defaulted
"""
self.minimum_fault_length_to_export = 500.0

self.minimum_fault_length_to_export = 0.0
Copy link
Member

Choose a reason for hiding this comment

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

should this be a parameter that can be initialised?

Copy link
Member

Choose a reason for hiding this comment

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

in case it's not user-defined, I think it would be better to automatically calculate the minimum fault length given the scale of the map

map2loop/mapdata.py Outdated Show resolved Hide resolved
map2loop/project.py Outdated Show resolved Hide resolved
map2loop/project.py Outdated Show resolved Hide resolved
map2loop/project.py Outdated Show resolved Hide resolved

thickness_combined_results = pandas.concat(thickness_calculation_results, axis=1)

self.stratigraphic_column.stratigraphicUnits.loc[:, 'ThicknessMean_ThicknessCalculatorAlpha'] = thickness_combined_results['ThicknessMean_ThicknessCalculatorAlpha']
Copy link
Member

Choose a reason for hiding this comment

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

Could you not just put the thickness_combined_results directly inside the stratigraphicUnits inside the for loop. This means you aren't hard coding the number of thickness calculators

Comment on lines +43 to +51
("ThicknessMean_ThicknessCalculatorAlpha", float),
("ThicknessMedian_ThicknessCalculatorAlpha", float),
("ThicknessStdDev_ThicknessCalculatorAlpha", float),
("ThicknessMean_InterpolatedStructure", float),
("ThicknessMedian_InterpolatedStructure", float),
("ThicknessStdDev_InterpolatedStructure", float),
("ThicknessMean_StructuralPoint", float),
("ThicknessMedian_StructuralPoint", float),
("ThicknessStdDev_StructuralPoint", float),
Copy link
Member

Choose a reason for hiding this comment

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

Is there a way to make this generic so that the user can specify thickness calculators to keep map2loop being modular?

Copy link
Member

@lachlangrose lachlangrose left a comment

Choose a reason for hiding this comment

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

I haven't reviewed or tested the code but I have made some suggestions within the code.

My main comment for this PR is that the changes to the thickness calculators have not been implemented in the modular framework of map2loop. Instead of setting the list of thickness calculators within the method calling the thickness calculators, the list of thickness calculators should be managed by the project class. The default list can be hard coded using the current three, but the user should be able to call set_thickness_calculators and provide a custom list of thickness calculators.

This would mean, if the thicknesss calculators have any parameters e.g. the threshold angle for the structure point, or the interpolator type for the interpolated structure these could be set by the user and the thickness calculator objects can be set.

@AngRodrigues AngRodrigues marked this pull request as draft August 12, 2024 01:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants