Namespaces

[lars-reimann]

Summary of Changes

Implement namespaces as described in #14.

Tasks

• Add concatenate_functions_tree
• Add create_input_structure_tree
• Add target parameter to concatenate_functions_tree
• Add missing docstrings
• Fix linter issues

[lars-reimann]

@janosg

Two somewhat related questions about the CI setup:

1. The step "Install core dependencies." doesn't use environment.yml. Instead dependencies are listed again in tox.ini. Is this intended? Should I add the dependencies I need for this PR to tox.ini as well?
2. environment.yml requires a Python version of at least 3.9. The CI matrix still tests for compatibility with 3.7 and 3.8, however. Is this intended? Should I change the code to only use features that were already available in 3.7?

[lars-reimann]

@hmgaudecker @janosg

While the PR is not quite ready for review (missing docstrings, linter issues), I'd appreciate some feedback about the current implementation since it slightly differs from the proposal:

1. The function create_input_structure_tree doesn't have a target argument. Instead it computes the input template for all functions. I didn't see the need to restrict it here since this happens later anyway in concatenate_functions_tree. If users manually define the input structure, they can still only pass the ones needed by the target functions to concatenate_functions_tree.
2. level_of_inputs has the values "top" and "namespace" instead of "top" and "bottom". In my opinion, "namespace" better captures the intention of this setting.
3. target is not a nested dict with lists or strings as leaves but a nested dict with None as leaves, identical to input_structure. This allows to describe cases that need values from some level and from a level below it, like

   {
     "f": None,
     "namespace": {
       "g": None,
     }
   }

   The output of the concatenated function is then the same as the target description, with the Nones replaced by the computed values.

Other than that, the current implementation seems to offer the requested functionality.

To get the build working here, I also need some input to #15 (comment).

[hmgaudecker]

@janosg

Two somewhat related questions about the CI setup:

1. The step "Install core dependencies." doesn't use [`environment.yml`](https://github.com/OpenSourceEconomics/dags/blob/main/environment.yml). Instead dependencies are listed again in [`tox.ini`](https://github.com/OpenSourceEconomics/dags/blob/main/tox.ini). Is this intended? Should I add the dependencies I need for this PR to [`tox.ini`](https://github.com/OpenSourceEconomics/dags/blob/main/tox.ini) as well?

2. [`environment.yml`](https://github.com/OpenSourceEconomics/dags/blob/main/environment.yml) requires a Python version of at least 3.9. The CI matrix still tests for compatibility with 3.7 and 3.8, however. Is this intended? Should I change the code to only use features that were already available in 3.7?

Janoś is travelling today, so let me quickly reply that this simply has been neglected with 99.9% probability. The best thing would be to just copy the estimagic setup (nearly identical to GETTSIM), adding 3.11 to the matrix.

[hmgaudecker]

Other than that, the current implementation seems to offer the requested functionality.

Excellent, great work as always! I only had a quick glance, but that whetted my appetite to take a deeper look 😄 Not possible right now, unfortunately.

While the PR is not quite ready for review (missing docstrings, linter issues), I'd appreciate some feedback about the current implementation since it slightly differs from the proposal:

1. The function create_input_structure_tree doesn't have a target argument. Instead it computes the input template for all functions. I didn't see the need to restrict it here since this happens later anyway in concatenate_functions_tree. If users manually define the input structure, they can still only pass the ones needed by the target functions to concatenate_functions_tree.

I am afraid we'll need that. Take GETTSIM as an example. There are perfectly sensible applications where you need only a couple of targets instead of the dozens (hundreds?) it can potentially produce. In that case, your number of required inputs typically also decreases by the same order of magnitude. For a convenience function returning the required inputs, not much is gained from sifting through dozens of lines vis a vis potentially using flat names with typos / a nested dict with a wrong structure.

2. level_of_inputs has the values "top" and "namespace" instead of "top" and "bottom". In my opinion, "namespace" better captures the intention of this setting.

Great idea.

3. target is not a nested dict with lists or strings as leaves but a nested dict with None as leaves, identical to input_structure. This allows to describe cases that need values from some level and from a level below it, like

      {
        "f": None,
        "namespace": {
          "g": None,
        }
      }

The output of the concatenated function is then the same as the target description, with the Nones replaced by the computed values.

Makes sense.

Thanks again!

[lars-reimann]

I am afraid we'll need that. Take GETTSIM as an example. There are perfectly sensible applications where you need only a couple of targets instead of the dozens (hundreds?) it can potentially produce. In that case, your number of required inputs typically also decreases by the same order of magnitude. For a convenience function returning the required inputs, not much is gained from sifting through dozens of lines vis a vis potentially using flat names with typos / a nested dict with a wrong structure.

The target parameter is added now.

[codecov]

Codecov Report

Merging #15 (43173e8) into main (645e7d5) will increase coverage by 1.24%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main      #15      +/-   ##
==========================================
+ Coverage   96.46%   97.71%   +1.24%     
==========================================
  Files           8       10       +2     
  Lines         453      699     +246     
==========================================
+ Hits          437      683     +246     
  Misses         16       16              

Impacted Files	Coverage Δ
src/dags/dag.py	95.12% <ø> (ø)
src/dags/__init__.py	100.00% <100.00%> (ø)
src/dags/dag_tree.py	100.00% <100.00%> (ø)
tests/test_dag_tree.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[janosg]

@janosg

Two somewhat related questions about the CI setup:

1. The step "Install core dependencies." doesn't use environment.yml. Instead dependencies are listed again in tox.ini. Is this intended? Should I add the dependencies I need for this PR to tox.ini as well?

2. environment.yml requires a Python version of at least 3.9. The CI matrix still tests for compatibility with 3.7 and 3.8, however. Is this intended? Should I change the code to only use features that were already available in 3.7?

The whole CI setup in dags is a bit outdated. Back then we used tox to set up environments with multiple python versions, now we do that directly in GitHub actions.

We usually follow these guidelines for supported Python version. Thus currently we would need 3.9 to 3.11.

I think we should separate the CI update from this PR. So I suggest you now just remove 3.7 and 3.8 (and maybe add 3.11). The rest can be done later.

[janosg]

Regarding the pre-commits: I exclude PT006 in estimagic because I think it is misguided. This might also apply to some of the others that cause problems for you.

[hmgaudecker]

Did not make it all the way through, but what I saw looked great!

Just a couple of clarifications/misunderstandings at this point.

[lars-reimann]

The changes you've requested so far should be integrated now.

[hmgaudecker]

Why did you close this?