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

default_values #190

Closed
wants to merge 13 commits into from
Closed

default_values #190

wants to merge 13 commits into from

Conversation

benjello
Copy link
Collaborator

@benjello benjello commented Oct 28, 2015

This a a non working tentative which tries to reimplement @AlexisEidelman default_value declaration
@gdementen if you have a bit of time to have look at it I would be very glad.
And if you think, that it I am in the wrong direction, please be blunt !
Thanks

@landscape-bot
Copy link

landscape-bot commented Oct 28, 2015

Code Health
Repository health decreased by 0.75% when pulling 045a689 on benjello:defaut_values into 7709f8f on liam2:master.

@@ -193,6 +200,7 @@ def from_table(cls, table, start=0, stop=None, buffersize=10 * 2 ** 20):
max_buffer_rows = buffersize // dtype.itemsize
numlines = stop - start
ca = cls.empty(numlines, dtype)
ca.default_values = default_values
Copy link
Member

@gdementen gdementen Oct 28, 2015

Choose a reason for hiding this comment

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

empty should accept an optional arg, and this line should use it instead

@gdementen
Copy link
Member

gdementen commented Oct 28, 2015

There are strange things in this proposition, but it's hard to tell what exactly without thinking hard about it and I am too tired to do it today and will probably not have time to do it for a few weeks.

@benjello
Copy link
Collaborator Author

benjello commented Oct 28, 2015

Thanks @gdementen, it is messy because it is mainly a merge with a few fix. I will try on my own for a moment.

@landscape-bot
Copy link

landscape-bot commented Oct 29, 2015

Code Health
Repository health decreased by 0.25% when pulling 7a31efc on benjello:defaut_values into 7709f8f on liam2:master.

@benjello
Copy link
Collaborator Author

benjello commented Oct 29, 2015

@gdementen : should I modify the get_missing_* by adding a default_values argument or should I add it only to ColumnArray.empty and the calls to this method in ColumnArray.from_table and ColumnArray.from_records ?

@gdementen
Copy link
Member

gdementen commented Oct 29, 2015

@benjello I don't know. You should see what works best. I would find it elegant if we could suppress the concept of missing value, and only have default values which themselves default to the current values we use for missing values depending on the type of field.

@benjello
Copy link
Collaborator Author

benjello commented Oct 29, 2015

It is somehow what we have done in openfisca and it is actually useful. I
will try but I may come with more questions ;-) so stop when you get bored

Le jeu. 29 oct. 2015 09:03, Gaëtan de Menten notifications@github.com a
écrit :

@benjello https://github.com/benjello I don't know. You should see what
works best. I would find it elegant if we could suppress the concept of
missing value, and only have default values which themselves default to the
current values we use for missing values depending on the type of field.


Reply to this email directly or view it on GitHub
#190 (comment).

@gdementen
Copy link
Member

gdementen commented Oct 29, 2015

I won't get bored, but I am low on time... On the other hand I want to avoid slowing you down or discouraging you from contributing to LIAM2.

@benjello
Copy link
Collaborator Author

benjello commented Oct 29, 2015

We are all low on time ;-)

Le jeu. 29 oct. 2015 09:17, Gaëtan de Menten notifications@github.com a
écrit :

I won't get bored, but I am low on time... On the other hand I want to
avoid slowing you down or discouraging you from contributing to LIAM2.


Reply to this email directly or view it on GitHub
#190 (comment).

@gdementen
Copy link
Member

gdementen commented Oct 29, 2015

FWIW, np.full is numpy1.8+, so we'd need to bump the requirements in INSTALL.

@landscape-bot
Copy link

landscape-bot commented Oct 29, 2015

Code Health
Repository health decreased by 0.64% when pulling 4931d95 on benjello:defaut_values into 7344808 on liam2:master.

@landscape-bot
Copy link

landscape-bot commented Oct 29, 2015

Code Health
Repository health decreased by 0.64% when pulling 79e1e29 on benjello:defaut_values into 7344808 on liam2:master.

@benjello
Copy link
Collaborator Author

benjello commented Oct 29, 2015

I introduced default_values everywhere the was a missing_* but still it the test do not pass. I confess I do not figure the entire process very well so i could use a hint about how to debug this kind of problem ;-)

@landscape-bot
Copy link

landscape-bot commented Oct 29, 2015

Code Health
Repository health decreased by 0.64% when pulling 40bac28 on benjello:defaut_values into 7344808 on liam2:master.

@gdementen
Copy link
Member

gdementen commented Oct 31, 2015

The line "defaulted_integer_variable: [99 99 99 ..., -1 -1 -1] done (0 ms elapsed)." makes me suspect the new borns do not use the default value, which means the thing is probably working as it should for the initial population but not for newborns. Did you implement it?

@benjello
Copy link
Collaborator Author

benjello commented Nov 2, 2015

Good catch @gdementen. Tests do pass now. I can add more tests if needed.
You should carefully review this PR, i was not always very confident about what i was modifying.
I still lack a clear vision of the global architecture.

@landscape-bot
Copy link

landscape-bot commented Nov 2, 2015

Code Health
Repository health decreased by 0.68% when pulling 3b05758 on benjello:defaut_values into 7344808 on liam2:master.

@gdementen
Copy link
Member

gdementen commented Nov 2, 2015

I will not have time for a in-depth review & merge for a couple of weeks, but in the mean time, could you;

  • provide documentation
  • change the default value for the default arguments to None instead of {} (and update the code accordingly). Using a mutable default value for an argument is asking for trouble.
  • ideally add a mention in the changelog at doc/usersguide/source/changes/version_0_11.rst.inc
    Thanks!

@benjello
Copy link
Collaborator Author

benjello commented Nov 2, 2015

I wil try to find some time for that.

@landscape-bot
Copy link

landscape-bot commented Nov 12, 2015

Code Health
Repository health decreased by 0.07% when pulling dab7c8b on benjello:defaut_values into 58bea98 on liam2:master.

The fields that are not present in the initial file can also be initialized to
a specific value by using the `default: some_default_value`. If not present they
are initialized to the default of the field type which are False for boolean and
0 for integer and float (see the *alive* variable in the example below).
Copy link
Member

@gdementen gdementen Nov 12, 2015

Choose a reason for hiding this comment

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

this is not True (or at least was not True -- and should not be). -1 for integer and 'nan' for float

Copy link
Member

@gdementen gdementen Nov 12, 2015

Choose a reason for hiding this comment

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

You do no speak about new(). In fact I think a separate paragraph and saying it applies to both new() and fields not present in the input bla bla... would be better, what do you think?

Copy link
Collaborator Author

@benjello benjello Nov 12, 2015

Choose a reason for hiding this comment

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

You are right. Will do that.

On Thu, Nov 12, 2015 at 10:43 PM Gaëtan de Menten notifications@github.com
wrote:

In doc/usersguide/source/model.rst
#190 (comment):

@@ -219,6 +219,10 @@ However, in practice, there are often some fields which are not present in the
input file. They will need to be calculated later by the model, and you need to
tell LIAM2 that the field is missing, by using initialdata: False in the
definition for that field (see the agegroup variable in the example below).
+The fields that are not present in the initial file can also be initialized to
+a specific value by using the default: some_default_value. If not present they
+are initialized to the default of the field type which are False for boolean and
+0 for integer and float (see the alive variable in the example below).

You do no speak about new(). In fact I think a separate paragraph and
saying it applies to both new() and fields not present in the input bla
bla... would be better, what do you think?


Reply to this email directly or view it on GitHub
https://github.com/liam2/liam2/pull/190/files#r44719011.

@landscape-bot
Copy link

landscape-bot commented Nov 12, 2015

Code Health
Repository health decreased by 0.07% when pulling 8535c9e on benjello:defaut_values into 58bea98 on liam2:master.

benjello added 2 commits Nov 16, 2015
Move changes to versions_0_11.rst.inc
# Conflicts:
#	liam2/tests/functional/simulation.yml
@landscape-bot
Copy link

landscape-bot commented Nov 16, 2015

Code Health
Repository health decreased by 0.07% when pulling 3e8083c on benjello:defaut_values into bfcae0a on liam2:master.

@landscape-bot
Copy link

landscape-bot commented Nov 16, 2015

Code Health
Repository health decreased by 0.07% when pulling 761324f on benjello:defaut_values into bfcae0a on liam2:master.

@gdementen gdementen self-assigned this Nov 20, 2015
@gdementen gdementen added this to the 0.11 milestone Nov 20, 2015
@landscape-bot
Copy link

landscape-bot commented Nov 20, 2015

Code Health
Repository health decreased by 0.17% when pulling e5b4198 on benjello:defaut_values into 9162627 on liam2:master.

@gdementen
Copy link
Member

gdementen commented Nov 20, 2015

Merged! Thanks a lot!

@gdementen gdementen closed this Nov 20, 2015
@benjello
Copy link
Collaborator Author

benjello commented Nov 20, 2015

I hope you checked it carefully since I was not always confortable with what I was modifying.

@gdementen gdementen changed the title WIP default_values default_values Nov 20, 2015
@gdementen
Copy link
Member

gdementen commented Nov 20, 2015

Don't worry, I did not merge it as-is. I did check & modify it. There is still a bug left, but I wanted to add a test case for it before fixing it.

@benjello benjello deleted the defaut_values branch Nov 24, 2015
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