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

Load winiks from disk #39

Merged
merged 3 commits into from
Nov 26, 2020
Merged

Load winiks from disk #39

merged 3 commits into from
Nov 26, 2020

Conversation

ThomasThelen
Copy link
Collaborator

@ThomasThelen ThomasThelen commented Nov 13, 2020

Fixes issue #29

Similar to the 'load resources from disk' pull request and follows the same format.

@zizroc I think I've got something wonky with either my test csv or the way I'm loading values from it. If you get the chance can you see if anything jumps out (run the winik_manager test to see the errors)? I'm still a bit unsure about NULL vs NA. If you think that those NA's that I switched from NULL should go back to NULL let me know and I'll revert that change!

Also note that I didn't load any sort of children from the test winik file, or define any there. Since we're working with csv files, it's weird when you have to map a one->many relationship ie children->123, 543, 23 because the commas represent different columns. We could semi-colon separate the children so that the csv file is still valid-but I see that more of a hack than anything.

Right now, the winik-child relationships are like a doubly linked list. You can traverse from a child to a parent, or a parent to a child. We can get rid of the children list in the winik object and there's still the linking from a winik to its parent via the mother_id and father_id slots so we're not losing any information in doing so, and it keeps things a bit cleaner. But I'm pretty impartial. Any thoughts?

@ohmannyy
Copy link
Collaborator

The NA should stay since a variable with NULL won't allow a change in value later on in the code. With the parent-child relationship, in the generation of population R script file, I had worked on, I didn't include children as an isolated list. I think the mother and father ID should be more than sufficient to be able to track the lineage of a person. No need to have an additional children list.

@ThomasThelen ThomasThelen merged commit 031cab5 into master Nov 26, 2020
@ThomasThelen ThomasThelen deleted the load_winiks branch November 26, 2020 19:12
@ThomasThelen ThomasThelen restored the load_winiks branch November 26, 2020 20:29
@ThomasThelen ThomasThelen deleted the load_winiks branch January 18, 2021 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants