reading default values from yml files #23

neuronflow · 2023-12-15T14:38:11Z

Signed-off-by: neuronflow [email protected]

Signed-off-by: neuronflow <[email protected]>

sarthakpati · 2023-12-15T15:11:08Z

ereg/registration.py

@@ -188,6 +188,7 @@ def update_parameters(self, config_file: Union[str, dict], **kwargs):
        )

        # this is taken directly from the sample_config.yaml
+        # TODO this is ugly we should probbaly read these defaults from a file?


There are pros and cons for both the mechanisms. If we are reading from a file, we need to ensure that the file is packaged in pypi in a manner that works across various platforms.

yes, this is solved :)

ah it was not commited yet, see commits below

Signed-off-by: neuronflow <[email protected]>

sarthakpati · 2023-12-15T16:25:29Z

ereg/registration.py

+    def _generate_default_parameters(self) -> dict:
+        defaults_file = os.path.normpath(
+            os.path.abspath(
+                __file__ + "configurations/default_config.yaml",


Suggested change

__file__ + "configurations/default_config.yaml",

__file__ + "configurations/sample_config.yaml",

No need for another config file at all.

then we need to move the other files to this location :)

sarthakpati · 2023-12-15T16:25:47Z

ereg/configurations/default_config.yaml

@@ -0,0 +1,95 @@
+# TODO adjust default parameters!


This file can be deleted.

yes, it is a placeholder. the files we want to include into the pkg should be subfolders of the ereg folder :)

sarthakpati · 2023-12-15T16:29:42Z

This PR also needs changes in the MANIFEST and pyproject.tml files in accordance with the documentation here (or something similar): https://setuptools.pypa.io/en/stable/userguide/datafiles.html

neuronflow · 2023-12-15T16:32:37Z

This PR also needs changes in the MANIFEST and pyproject.tml files in accordance with the documentation here (or something similar): https://setuptools.pypa.io/en/stable/userguide/datafiles.html

should work without a manifest, at least in brainles preprocessing we also include files without a manifest [I believe the reason it works is that we use poetry]

sarthakpati · 2023-12-15T16:33:13Z

This PR also needs changes in the MANIFEST and pyproject.tml files in accordance with the documentation here (or something similar): https://setuptools.pypa.io/en/stable/userguide/datafiles.html

should work without a manifest, at least in brainles preprocessing we also include files without a manifest [I believe the reason it works is that we use poetry]

Oh okay. Should be fine as long as we can verify this without having to push a new version to pip.

neuronflow · 2023-12-15T16:34:27Z

This PR also needs changes in the MANIFEST and pyproject.tml files in accordance with the documentation here (or something similar): https://setuptools.pypa.io/en/stable/userguide/datafiles.html

should work without a manifest, at least in brainles preprocessing we also include files without a manifest [I believe the reason it works is that we use poetry]

Oh okay. Should be fine as long as we can verify this without having to push a new version to pip.

hmm if it works with pip install -e . and the defaults are loaded it should be fine?

sarthakpati · 2023-12-15T16:37:21Z

This PR also needs changes in the MANIFEST and pyproject.tml files in accordance with the documentation here (or something similar): https://setuptools.pypa.io/en/stable/userguide/datafiles.html

should work without a manifest, at least in brainles preprocessing we also include files without a manifest [I believe the reason it works is that we use poetry]

Oh okay. Should be fine as long as we can verify this without having to push a new version to pip.

hmm if it works with pip install -e . and the defaults are loaded it should be fine?

Cool! Move the sample_config and default_rigid files under configurations, remove the new one and we should be good!

neuronflow · 2023-12-15T16:37:28Z

currently we pkg everything that is under the ereg folder, I have to check whether .yml files are excluded, but I don't think so

Signed-off-by: neuronflow <[email protected]>

neuronflow · 2023-12-15T16:45:50Z

@sarthakpati so @IsraMekki0 and I went through the code today, we added this property:

eReg/ereg/registration.py

Line 83 in e4973e4

@property

This enables users to change the configuration of a registrator instance with
registrator.configration = "new_config.yml"
(before this might have lead to unexpected behavior)

Further, we added the possibility to read in defaults from the .yml

now what seems missing is that the update_parameters method only updates the parameters which are contained in the supplied dict. I don't think this method should supply defaults, this way we would have a single source of truth (the config file) file?

eReg/ereg/registration.py

Line 99 in e4973e4

    
           # TODO rewrite this function to only update specific entries which are given in the dict

sarthakpati · 2023-12-15T16:48:30Z

Sounds great! Thanks, @IsraMekki and @neuronflow

Signed-off-by: neuronflow <[email protected]>

neuronflow · 2023-12-15T22:41:26Z

ereg/registration.py

-            "bias_correct", self.parameters.get("bias", False)
-        )
-        self.parameters["interpolator"] = (
-            self.parameters.get("interpolator", "linear")


I don't fully understand why this is done @sarthakpati . The

.replace("_", "") .replace("-", "") .lower()

@sarthakpati did you see this?

Uh, I think this is to just remove these specific special characters to make the comparisons easier in the next lines.

add todo for later

64a908a

Signed-off-by: neuronflow <[email protected]>

neuronflow marked this pull request as draft December 15, 2023 14:38

sarthakpati reviewed Dec 15, 2023

View reviewed changes

neuronflow added 3 commits December 15, 2023 16:26

reading defaults from file

4547d52

Signed-off-by: neuronflow <[email protected]>

new: add todo

07256d5

Signed-off-by: neuronflow <[email protected]>

gets rid of auxiliary requirement to still allow python 3.8 legacy users

03b09b1

Signed-off-by: neuronflow <[email protected]>

neuronflow mentioned this pull request Dec 15, 2023

Updated default parameters from original repo #24

Merged

Merge branch 'main' into add_config_file_property

3e3f413

neuronflow changed the title ~~add todo for later~~ reading default values from yml files Dec 15, 2023

sarthakpati reviewed Dec 15, 2023

View reviewed changes

ymls

e4973e4

Signed-off-by: neuronflow <[email protected]>

neuronflow and others added 4 commits December 15, 2023 22:58

change name of yml

2669a17

Signed-off-by: neuronflow <[email protected]>

Merge branch 'main' into add_config_file_property

65f063b

be consistent

fc92336

Signed-off-by: neuronflow <[email protected]>

hmm this should update the dict

896af5e

Signed-off-by: neuronflow <[email protected]>

neuronflow commented Dec 15, 2023

View reviewed changes

neuronflow added enhancement New feature or request help wanted Extra attention is needed labels Dec 15, 2023

neuronflow assigned neuronflow, sarthakpati and IsraMekki0 Dec 15, 2023

neuronflow linked an issue Dec 15, 2023 that may be closed by this pull request

[feature request] we want to have a single source of truth for default parameters #26

Closed

Merge branch 'main' into add_config_file_property

032545b

neuronflow marked this pull request as ready for review March 27, 2024 20:44

sarthakpati approved these changes Mar 27, 2024

View reviewed changes

sarthakpati enabled auto-merge March 27, 2024 20:46

sarthakpati merged commit aedc922 into main Mar 27, 2024
6 checks passed

sarthakpati deleted the add_config_file_property branch March 27, 2024 20:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

reading default values from yml files #23

reading default values from yml files #23

neuronflow commented Dec 15, 2023

sarthakpati Dec 15, 2023

neuronflow Dec 15, 2023

neuronflow Dec 15, 2023

sarthakpati Dec 15, 2023

neuronflow Dec 15, 2023

sarthakpati Dec 15, 2023

sarthakpati Dec 15, 2023

neuronflow Dec 15, 2023

sarthakpati commented Dec 15, 2023

neuronflow commented Dec 15, 2023

sarthakpati commented Dec 15, 2023

neuronflow commented Dec 15, 2023

sarthakpati commented Dec 15, 2023

neuronflow commented Dec 15, 2023

neuronflow commented Dec 15, 2023

sarthakpati commented Dec 15, 2023

neuronflow Dec 15, 2023 •

edited

Loading

neuronflow Mar 28, 2024

sarthakpati Mar 28, 2024

	__file__ + "configurations/default_config.yaml",
	__file__ + "configurations/sample_config.yaml",

reading default values from yml files #23

reading default values from yml files #23

Conversation

neuronflow commented Dec 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarthakpati commented Dec 15, 2023

neuronflow commented Dec 15, 2023

sarthakpati commented Dec 15, 2023

neuronflow commented Dec 15, 2023

sarthakpati commented Dec 15, 2023

neuronflow commented Dec 15, 2023

neuronflow commented Dec 15, 2023

sarthakpati commented Dec 15, 2023

neuronflow Dec 15, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neuronflow Dec 15, 2023 •

edited

Loading