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

Dev/move deps to interdependencies #1477

Conversation

WilliamHPNielsen
Copy link
Contributor

@WilliamHPNielsen WilliamHPNielsen commented Feb 15, 2019

This PR may replace #1438

The idea is to move all information about how parameters are related to each other into a new InterDependencies object and thus out of the the ParamSpec.

Changes proposed in this pull request:

  • Make ParamSpec only have four attributes: name, datatype, label, unit
  • Make InterDependencies take three input arguments: dependencies, inferences, and standalones
  • Make the type of those arguments do most of the validation (nice, huh?)

All validation takes place upon instantiating an InterDependencies object, i.e. if it exists, it is valid.

You can get an impression of how this will work by looking at the ParamSpecBase and the InterDependencies_ objects of this PR. If you like it, we can begin to discuss how to proceed with rolling this out, deprecating old things etc. I have a plan for that, but let's first decide whether we like this design.

Open questions:

  • How should the new InterDependencies object serialize itself? Do we need a DB upgrade?
  • Are the rules imposed by InterDependencies_ for how parameters may relate to each other the correct rules. I think they are, but I'd like your input on that.

@QCoDeS/core

AB#2861

@WilliamHPNielsen
Copy link
Contributor Author

@QCoDeS/core, a question that this PR is now facing is: what does it take to uniquely determine a parameter in a run? Up until now, we have imposed in the Measurement interface that the name of each parameter must be unique, mainly (I think) because that is how ParamSpec list their dependencies. At the same time, we use the full_name internally in the SQLite database to refer to parameter values, both in the parameters column of the runs table and as columns in the results_table. When retrieving data from the DB, the values we get out are thus also indexed by the full_name.

I think it would be good if we could manage to use the same identifier everywhere, that is to say that we use whatever thing must be unique as the parameter identifier. Also, I feel that unique names are somewhat too restrictive. The name-label-unit combination being unique feels more natural?

Now that the InterDependencies_ object serializes itself into a lookup table of {ID: serialized paramspec}, it might make sense to use that very same ID as the internal SQLite identifier? In that case, the ID should certainly be something more readable than the hex(hash(ParamSpecBase)) that my latest commit uses!

I realise that this is potentially a lot of effort for a rather small payoff, but I wonder if having and using some "formal" ID would not benefit us in the long run?

I guess I can condense this stream of consciousness down to two questions:

  1. Should we not use [whatever it is that must be unique] as column names and internal reference to parameters? (i.e. not require that name be unique, but actually give back data indexed by full_name)?
  2. What should that unique ID be? name is one option, name+label+unit is another, the hash is a third, but there are many more.

@astafan8
Copy link
Contributor

ideally, "some ID" should identify a parameter within a run, where parameter is represented via ParamSpec which is just an bag with name,label,unit. IDs can be specific to the scope/context (one ID within sqlite, other ID within interdeps), or global -- i don't know yet what's the "good enough".

Since going to something like this takes some effort and some db upgrades, i think it's definitely worth doing, but we can take small steps. So, just to suggest the first step: introduce IDs that are hex(hash()) to interdeps, but keep using names (or full names) in sqlite.

@codecov
Copy link

codecov bot commented Apr 3, 2019

Codecov Report

Merging #1477 into master will increase coverage by 0.71%.
The diff coverage is 95.46%.

@@            Coverage Diff             @@
##           master    #1477      +/-   ##
==========================================
+ Coverage   70.52%   71.23%   +0.71%     
==========================================
  Files         103      103              
  Lines       11608    11994     +386     
==========================================
+ Hits         8186     8544     +358     
- Misses       3422     3450      +28

Copy link
Collaborator

@jenshnielsen jenshnielsen left a comment

Choose a reason for hiding this comment

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

Mainly some smallish suggestions from the first read through

qcodes/dataset/dependencies.py Outdated Show resolved Hide resolved
old_error, deps_error[1])

inffs_error = self.validate_paramspectree(inferences)
if not inffs_error is None:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if not inffs_error is None:
if inffs_error is not None:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

qcodes/tests/dataset/test_dependencies.py Show resolved Hide resolved
qcodes/dataset/dependencies.py Show resolved Hide resolved
qcodes/dataset/dependencies.py Show resolved Hide resolved
qcodes/dataset/dependencies.py Show resolved Hide resolved
The test suite error chain detection has been
improved, so we can remove the clonky
raise-catch-reraise from helper function

@pytest.mark.usefixtures("experiment")
@pytest.mark.parametrize("storage_type", ['numeric', 'array'])
def test_datasaver_arrays_of_different_length(storage_type):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's hammer this a bit with hypothesis.

@WilliamHPNielsen WilliamHPNielsen merged commit 6b8f4d1 into microsoft:master Apr 17, 2019
@WilliamHPNielsen WilliamHPNielsen deleted the dev/move_deps_to_interdependencies branch April 17, 2019 10:32
giulioungaretti pushed a commit that referenced this pull request Apr 17, 2019
Merge: 4f34fdd 1d9cfc3
Author: William H.P. Nielsen <whpn@mailbox.org>

    Merge pull request #1477 from WilliamHPNielsen/dev/move_deps_to_interdependencies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants