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

Refactor lwt and add force_64 option #1590

Merged
merged 2 commits into from
Jul 29, 2021

Conversation

jeromekelleher
Copy link
Member

Closes #1589

@jeromekelleher jeromekelleher marked this pull request as draft July 26, 2021 06:00
@jeromekelleher jeromekelleher force-pushed the force-64-lwt branch 2 times, most recently from c0f3916 to 5b3869d Compare July 28, 2021 08:31
@codecov
Copy link

codecov bot commented Jul 28, 2021

Codecov Report

Merging #1590 (df59a4f) into main (5256bf7) will increase coverage by 0.01%.
The diff coverage is 97.29%.

❗ Current head df59a4f differs from pull request most recent head 0ef9b51. Consider uploading reports for the commit 0ef9b51 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1590      +/-   ##
==========================================
+ Coverage   93.60%   93.62%   +0.01%     
==========================================
  Files          27       27              
  Lines       23036    23074      +38     
  Branches     1079     1079              
==========================================
+ Hits        21562    21602      +40     
+ Misses       1440     1438       -2     
  Partials       34       34              
Flag Coverage Δ
c-tests 91.85% <ø> (ø)
lwt-tests 93.40% <97.29%> (+0.43%) ⬆️
python-c-tests 95.33% <97.29%> (+0.03%) ⬆️
python-tests 98.80% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
python/lwt_interface/tskit_lwt_interface.h 95.41% <97.29%> (+0.36%) ⬆️
python/_tskitmodule.c 91.84% <0.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5256bf7...0ef9b51. Read the comment docs.

@jeromekelleher jeromekelleher marked this pull request as ready for review July 29, 2021 05:24
Copy link
Member

@benjeffery benjeffery left a comment

Choose a reason for hiding this comment

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

Looks good, refactoring is an improvement! I assume we don't need an issue to track the logic flip as it is going to error out.

PyArrayObject *offset_array = NULL;
bool offset_64 = force_offset_64; // || col->offset[col->num_rows] > UINT32_MAX
int offset_type = offset_64 ? NPY_UINT64 : NPY_UINT32;
/* TODO change this to 32 bit when we flip tsk_size_t over */
Copy link
Member

Choose a reason for hiding this comment

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

By this comment you mean the whole logic of this section will be flipped? I think I get it!

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, exactly.

@benjeffery benjeffery added the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Jul 29, 2021
@mergify mergify bot merged commit 966e524 into tskit-dev:main Jul 29, 2021
@mergify mergify bot removed the AUTOMERGE-REQUESTED Ask Mergify to merge this PR label Jul 29, 2021
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.

Add force_offset_64 option to LightweightTableCollection.asdict()
2 participants