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

change attribut group to dictionary #437

Merged
merged 3 commits into from Mar 8, 2018

Conversation

Projects
None yet
5 participants
@uvchik
Copy link
Member

uvchik commented Jan 15, 2018

With the negative/positive gradient we added dictionaries as attributes to the Flow class.

In the constructor they are named "sequences". I just stumbled over it searching for something else, but naming types correctly make things clearer.

If we have more dictionaries in the future we can distinguish between them within the if statement:

if attribute in dictionaries:
    if attribute in typ_a_dictionary:
        do something
    elif attribute in typ_b_dictionary:
        do something else
elif:
    ....

As long as we do have just one type of dictionaries the second if statement is left out.

@uvchik uvchik requested review from simnh and ckaldemeyer Jan 15, 2018

@uvchik uvchik self-assigned this Jan 15, 2018

@uvchik uvchik added this to the v0.2.1 milestone Jan 15, 2018

@simnh

This comment has been minimized.

Copy link
Member

simnh commented Jan 15, 2018

I would approve this one, but can we finalize this one first (together with it) #405

@henhuy

This comment has been minimized.

Copy link
Contributor

henhuy commented Jan 15, 2018

That was something @ckaldemeyer and I also stumbled upon while creating the param_results in #416.
First, those were added to sequences, as it was an iterable - then we recursively flattened all dicts, so that {'negative_gradient': {'ub': x, 'costs': y}} became {'negative_gradient_ub': x, 'negative_gradient_costs': y} - and finally looked through all sequences and moved "false"-sequences into scalars-dict.
Now considering your suggestion, maybe it makes sense to add an additional key dictionaries to the param_results dict?!

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Jan 15, 2018

My commits do not change anything, I just renamed the attribute-groups, because they were named sequences but they were dictionaries.

Furthermore, we could of course discuss the idea of dictionary-attributes but this was not the intention of this PR.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Jan 16, 2018

@simnh You can set the review on hold with "request changes" and approve it when #405 is merged. In that way it is visible for everybody.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Jan 17, 2018

positive_gradient : dictionary
       Two obligate keys:
       'ub': numeric (sequence, scalar or None), the normed maximal positive
        difference (flow[t-1] < flow[t]) of two consecutive flow values
        (ub = upper bound).
       'costs': numeric (scalar or None), the gradient cost per unit.
negative_gradient : dictionary
       Two obligate keys:
       'ub': numeric (sequence, scalar or None), the normed maximal negative
        difference (flow[t-1] > flow[t]) of two consecutive flow values
        (ub = upper bound).
        'costs': numeric (scalar or None), the gradient cost per unit.

I revised the docstring. Does it look okay for you.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Jan 18, 2018

@simnh Don't you think we should merge this into dev and into #405.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Feb 12, 2018

This is more or less a fix and does not have effect on the output. Shouldn't we just merge it?

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Feb 20, 2018

@simnh I would like to merge this PR for v0.2.1. I still think we could merge it before #405.

@jnnr

This comment has been minimized.

Copy link
Contributor

jnnr commented Mar 5, 2018

I had a look at the code. In my opinion, it is a good idea to move the gradients out of the sequences list. However, why create a new dictionary and not put it into scalars? Maybe you have a reason for that which I do not see now. In my opinion, gradients are attributes that are belong in the same category as nominal_value.

Any other comments on this PR before the release of v0.2.1?

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Mar 5, 2018

It is kind of "blocked" by @simnh but he has not reacted for seven weeks so maybe we just merge it if nothing happens the next days.

@uvchik

This comment has been minimized.

Copy link
Member Author

uvchik commented Mar 7, 2018

According to @henhuy's comment we may discuss if and how we want dictionaries as attributes, but anyway this PR makes the actual state clearer.

I think we should merge this PR and open an issue (discussion) about dictionaries as attributes. I might be confusing if a dictionary attributes ends in separated attributes in the param_results. We should decide which way to go and implement it consistently.

@jnnr

This comment has been minimized.

Copy link
Contributor

jnnr commented Mar 7, 2018

Any comments? Should we merge this?

@uvchik uvchik merged commit 0d0a938 into dev Mar 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@uvchik uvchik deleted the revise/clean_attributes_of_flow_class branch Mar 8, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment