Issue34 dependent parameters #112

damskii9992 · 2025-04-10T08:31:59Z

No description provided.

…ters

github-actions

This pull request does not contain a valid label. Please add one of the following labels: ['chore', 'fix', 'bugfix', 'bug', 'enhancement', 'feature', 'dependencies', 'documentation']

damskii9992 · 2025-04-10T08:33:52Z

Have to add a deprecated label for the CI to proceed

damskii9992 · 2025-04-10T09:29:29Z

Hmm, the _process_dependency_symbol_names actually does not work, because the output of globals() from a module yields the modules global scope, rather than the users. I do not see a solution to this problem and will instead try out an implementation with a dependency_map dictionary input

rozyczko

General points:

complexity: AST parsing and asteval.Interpreter pose significant complexity, making the code potentially harder to understand and maintain.
performance: the observer pattern and dependency evaluation could introduce performance overhead, especially in scenarios with many dependent parameters or frequent updates. We need to run benchmarks to see if the overhead is not too serious
error handling: too much reliant on exceptions, which can clutter logs, cause unwanted exits or fail to provide clear error messages in complex setups

Additionally, complex methods like _process_dependency_symbol_names and _process_dependency_unique_names need clear examples for developers to understand their usage.

rozyczko · 2025-04-14T12:30:38Z

src/easyscience/Objects/variable/parameter.py

        :param description: A brief summary of what this object is
        :param url: Lookup url for documentation/information
        :param display_name: The name of the object as it should be displayed
-        :param enabled: Can the objects value be set
+        :param independent: Is the object dependent on another object?


We use the enabled state quite extensively in the libs/guis. Why did you remove this attribute?

Well, can you tell me what its purpose is?
How does it differ at all from "fixed" or (not) "independent"?
In all the code I could see about it, it looked like it was just an alias for not independent.

rozyczko · 2025-04-14T12:51:18Z

src/easyscience/Objects/variable/parameter.py

+            self._scalar.variance = temporary_parameter.variance
+            self._min.value = temporary_parameter.min
+            self._max.value = temporary_parameter.max
+            self._notify_observers()


The only occurrence of this method being called is in DescriptorNumber. It is called from _notify_observers() method.
Does this mean the self._notify_observers() called here will be also called the DescriptorNumber and cause an infinite loop?

Nope. Remember that Parameter inherits from DescriptorNumber. The only reason the _notify_observers() methods is written in the DescriptorNumber code, is because a Parameter should be able to depend on a DescriptorNumber.
Currently there can be an infinite loop if a dependent parameter is dependent on another dependent parameter which relies on itself, since each of those parameters have the other parameter in their _observers list, but this will be handled by my unique update id suggestion

rozyczko · 2025-04-14T12:52:45Z

src/easyscience/Objects/variable/parameter.py

+            # Notify observers of the change
+            self._notify_observers()
+        else:
+            raise AttributeError("This parameter is not independent, its value cannot be set directly. Please make it independent first.")  # noqa: E501


Do we want to raise here? (And the methods below).
Maybe just notify the user and drop the assignment attempt.

Well, we raise when the user tries to set the full value. Since that is a read-only property.
For a dependent parameter, the value (and other entries) are also read-only properties, so I figured they should be handled in the same way. We could also raise a warning, but trying to set the value of a dependent parameter is an error, and that is why I flag it as such.

damskii9992 · 2025-04-14T13:29:02Z

General points:

complexity: AST parsing and asteval.Interpreter pose significant complexity, making the code potentially harder to understand and maintain.

As I mentioned in my own comment, the AST parsing didn't work anyways due to the global namescope not being available inside modules, so I am currently removing it in favour of a simpler (albeit more verbose) approach where the user has to pass a dictionary of the dependencies along with the string to create a dependent parameter (unless unique_names are used).

performance: the observer pattern and dependency evaluation could introduce performance overhead, especially in scenarios with many dependent parameters or frequent updates. We need to run benchmarks to see if the overhead is not too serious

Yes, I think this is an issue we would run into with any implementation of dependent parameters, including our current one, but we will definitely need to benchmark it. Especially with the approach I am currently implementing where each Parameter gets its own asteval interpreter, I am concerned about the potential performance issue as I am unsure of how heavy the asteval interpreter object is.

error handling: too much reliant on exceptions, which can clutter logs, cause unwanted exits or fail to provide clear error messages in complex setups

What do you mean? Most of the error handling I do is specifically aimed at providing clearer error messages when stuff fails.

Additionally, complex methods like _process_dependency_symbol_names and _process_dependency_unique_names need clear examples for developers to understand their usage.

I was gonna go through the implementation of the _process_dependency_symbol_names method in the ADR suggestion due to its complexity, with an example, before that implementation idea was scratched.
Is the _process_dependency_unique_names a complicated method? I find it pretty self-explanatory, but maybe I've gotten to used to my own code to see it?

…han from the global scope

…cies

…ve dependent parameter constructor to class method

elindgren

I had a look at the current implementation. I think it is very readable actually, and it seems like a lot of cases have been handled w.r.t cleanup etc. My main concern is performance because of ASTeval, but I don't see a better way of doing things. Would be interesting to see what the computational cost is for a real-life example.

src/easyscience/Constraints.py

elindgren · 2025-04-24T09:12:56Z

src/easyscience/Objects/variable/descriptor_number.py

+    @notify_observers
+    def convert_unit(self, unit_str: str) -> None:
+        """


This is the user-facing version of convert_unit, right? So from the user's perspective nothing has changed API-wise w.r.t converting units?

Yes, exactly. Nothing has changed from the user-perspective.

src/easyscience/Objects/variable/descriptor_number.py

src/easyscience/Objects/variable/parameter.py

elindgren · 2025-04-24T09:24:49Z

src/easyscience/Objects/variable/parameter.py

+        clean_dependency_string = dependency_expression
+        existing_unique_names = self._global_object.map.vertices()
+        # Add the unique names of the parameters to the ASTEVAL interpreter
+        for name in inputted_unique_names:


inputted_unique_names only contains global ids, right? So if no global id's are passed in the expression this will be skipped?

Yup. If there are no unique_names in the dependency expression, inputted_unique_names will be empty and therefore the for-loop will not run.

src/easyscience/Objects/variable/descriptor_number.py

rozyczko · 2025-04-24T11:20:08Z

src/easyscience/Objects/variable/descriptor_number.py

@@ -115,6 +154,7 @@ def value(self) -> numbers.Number:
        return self._scalar.value

    @value.setter
+    @notify_observers
    @property_stack_deco


This decorator should really be renamed to property_stack only. _deco looks weird.
Unrelated but annoying

Yes, I agree. We can sneak that change into this PR :D

…s in the dependent parameters.

rozyczko · 2025-04-25T13:36:44Z

docs/src/fitting/constraints.rst

Consider replacing rather than deleting the documentation on constraints.
It will be required to have this available and the best time to write the docs is when the feature is implemented...

I don't think these docs actually work or are published anywhere. At least the contents does not appear to be similar to https://easyscience.github.io/corelib/modules/constraints/

In fact, should I just remove the entire docs folder? There is basically nothing in it.
I still don't know exactly what our documentation plans are.

damskii9992 added 10 commits March 5, 2025 13:25

first changes

5d62a99

Merge branch '10-dependent-parameters' into issue111_Dependent_Parame…

8b319d5

…ters

Add Observer pattern to DescriptorNumber

e27d595

rename and move observer methods

df1d138

Add the dependency_interpreter to the global_object

7b61c5c

implement the _update method

6fd5d7a

Update constructor to accept strings as values

2d41091

Disable setters when parameter is dependent

78532cd

Allow unique_names in dependency expression

5ec44a5

Allow symbols from the global scope in the dependency expression

cb4d2c3

github-actions bot requested changes Apr 10, 2025

View reviewed changes

damskii9992 added [scope] significant Breaking or major changes (MAJOR.minor.patch) dependencies Pull requests that update a dependency file, PR label labels Apr 10, 2025

github-actions bot approved these changes Apr 10, 2025

View reviewed changes

damskii9992 linked an issue Apr 10, 2025 that may be closed by this pull request

Replace constraints with dependent Parameters #34

Open

rozyczko reviewed Apr 14, 2025

View reviewed changes

damskii9992 added 5 commits April 15, 2025 11:48

Change symbol names to come from a dependency_map dict input rather t…

addac0c

…han from the global scope

Allow dependencies of dependent parameters. Check for cyclic dependen…

047568c

…cies

Make method for making a parameter dependent after initialization. Mo…

878293c

…ve dependent parameter constructor to class method

Add make_dependent method

51bd9c4

Add dependency property getters

b285ef1

elindgren reviewed Apr 24, 2025

View reviewed changes

rozyczko reviewed Apr 24, 2025

View reviewed changes

src/easyscience/Objects/variable/descriptor_number.py Show resolved Hide resolved

rozyczko reviewed Apr 24, 2025

View reviewed changes

src/easyscience/Objects/variable/descriptor_number.py Show resolved Hide resolved

rozyczko reviewed Apr 24, 2025

View reviewed changes

Remove old constraints and fix failing unit tests. Also fix small bug…

e3fad22

…s in the dependent parameters.

rozyczko reviewed Apr 25, 2025

View reviewed changes

damskii9992 added 3 commits April 25, 2025 15:38

Fix integration tests

9a770fe

rename property_stack_deco and make_dependent

30f19e3

Pr comments

0115eb7

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue34 dependent parameters #112

Issue34 dependent parameters #112

damskii9992 commented Apr 10, 2025

github-actions bot left a comment

damskii9992 commented Apr 10, 2025

damskii9992 commented Apr 10, 2025

rozyczko left a comment

rozyczko Apr 14, 2025

damskii9992 Apr 14, 2025

rozyczko Apr 14, 2025

damskii9992 Apr 14, 2025

rozyczko Apr 14, 2025

damskii9992 Apr 14, 2025

damskii9992 commented Apr 14, 2025

elindgren left a comment

elindgren Apr 24, 2025

damskii9992 Apr 24, 2025

elindgren Apr 24, 2025

damskii9992 Apr 24, 2025

rozyczko Apr 24, 2025

damskii9992 Apr 24, 2025

rozyczko Apr 25, 2025

damskii9992 Apr 25, 2025

damskii9992 Apr 25, 2025

Issue34 dependent parameters #112

Are you sure you want to change the base?

Issue34 dependent parameters #112

Conversation

damskii9992 commented Apr 10, 2025

github-actions bot left a comment

Choose a reason for hiding this comment

damskii9992 commented Apr 10, 2025

damskii9992 commented Apr 10, 2025

rozyczko left a 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

damskii9992 commented Apr 14, 2025

elindgren left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment