-
Notifications
You must be signed in to change notification settings - Fork 28
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
Anglo saxon kinship #138
Anglo saxon kinship #138
Conversation
This is on my desk to review for now. |
OK, so I've now gone through Table S1 of Gretzinger et al. 2022 and added all relationships except for Buckland (BUK). @93Boy, please go through BUK and add the relationships, you can take a look at the other sites how I have encoded things. here are a few best-practices that I've found:
Let me know if you run into problems. Otherwise please finalise the BUK relationships and mark this PR as "Ready for review" when you're done. Thanks! |
@stschiff I have added the BUK relationships but when I try to rectify the file afterward, trident throws this error |
I'll wait for your green light to review this again, @93Boy. Or just click "Ready for review", please |
I have gone through multiple consistency issues in this file and I have noticed the file structure is corrupted and data fields have been severely mixed up. I tried to retrieve the data from the master branch but seems like the master branch also has the same mixed-up file structure. I appreciate you having a look and giving a guideline. Another technical issue is you have categorized several relationships as a third degree but trident support only |
OK I will take a look whether I can see what's going on. Regarding the third-degree relationships: We discussed this in the meeting: You should then indeed set |
OK, @93Boy, I have now fixed the structural issue. When you run
These issues are caused by inconsistencies between the Relationship-Columns, where you have different number of list-items in the different columns. Please take a look and fix. |
Reminder, that this is on your desk, @93Boy, unless I'm told otherwise. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool that this is ready now! Very impressive!
I only see some details:
- The package version now jumps from v1.0.0 to v1.2.0. I think v1.1.0 would be a bit more consistent.
- The changelog message "Relationship_update" is pretty short and technical for such an amazing piece of work.
- This package is actually broken, there are a handful of these warnings
[Warning] [2024-03-08 08:31:05] Issue in data/2022_Gretzinger_AngloSaxons/2022_Gretzinger_AngloSaxons.janno in line 23 (Poseidon_ID: HID001.A): Date_Type is "C14", but either Date_C14_Uncal_BP or Date_C14_Uncal_BP_Err are empty
This PR may be a good place to quickly fix this. In this case the version could be incremented to v1.1.1, I think.
What the... I actually ran trident validate. How could I overlook this. OK, I'll take care of this. |
OK, I've fixed all the points raised by @nevrome except for the broken C14 lines which I'm gonna fix now as well. |
OK, done. I've fixed all the errors. I will add one issue, as the C14-data here can't be quite correct for one line. But the validation should now pass. |
Looks good to me now. Can be merged 👍 |
this is a branch that I recreated after prematurely having merged #136. I will likely merge #104 soon, but more work is needed on this PR to get it done.