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

Fix inefficient and sometimes inaccurate build process #451

Merged
merged 33 commits into from
Nov 7, 2020
Merged

Conversation

rly
Copy link
Contributor

@rly rly commented Nov 6, 2020

Motivation

Fix #337 and fix NeurodataWithoutBorders/pynwb#1313

This moves the setting of reference builders to the end of the build process, similar to how in h5tools.py, the writing of references is done at the end of the writing process. This ensures that all reference targets are built when they are encountered while traversing the build hierarchy, NOT when they are the target of a reference, which can lead to inefficient and sometimes inaccurate builds, especially with DynamicTables.

Now BuildManager.build(...) which must have the new argument root=True when building the root container in the hierarchy.

This also adds a few missing tests for the setting of datasets of references, compound datasets with references, and untyped datasets with references.

This also handles the rare case when a Container data type does not match the data type described in the spec.

This also fixes a floating point precision issue with TestCase.assertContainerEqual(...)

Checklist

  • Did you update CHANGELOG.md with your changes?
  • Have you checked our Contributing document?
  • Have you ensured the PR clearly describes the problem and the solution?
  • Is your contribution compliant with our coding style? This can be checked running flake8 from the source directory.
  • Have you checked to ensure that there aren't other open Pull Requests for the same change?
  • Have you included the relevant issue number using "Fix #XXX" notation where XXX is the issue number? By including "Fix #XXX" you allow GitHub to close issue #XXX when the PR is merged.

@codecov
Copy link

codecov bot commented Nov 6, 2020

Codecov Report

Merging #451 (ae08f81) into dev (f4b8232) will increase coverage by 0.27%.
The diff coverage is 90.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #451      +/-   ##
==========================================
+ Coverage   81.03%   81.30%   +0.27%     
==========================================
  Files          36       36              
  Lines        7365     7425      +60     
  Branches     1617     1618       +1     
==========================================
+ Hits         5968     6037      +69     
+ Misses       1024     1016       -8     
+ Partials      373      372       -1     
Impacted Files Coverage Δ
src/hdmf/build/builders.py 87.65% <0.00%> (-0.24%) ⬇️
src/hdmf/build/objectmapper.py 83.78% <88.75%> (+1.79%) ⬆️
src/hdmf/backends/io.py 94.64% <100.00%> (ø)
src/hdmf/build/__init__.py 100.00% <100.00%> (ø)
src/hdmf/build/errors.py 100.00% <100.00%> (ø)
src/hdmf/build/manager.py 78.95% <100.00%> (+0.90%) ⬆️

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 f4b8232...ae08f81. Read the comment docs.

@bendichter
Copy link
Contributor

Wow, looks like a lot of work. Much appreciated!

@rly rly marked this pull request as ready for review November 6, 2020 09:02
@rly rly requested a review from a team November 6, 2020 09:03
@rly rly changed the title [WIP] Fix inefficient and sometimes inaccurate build process Fix inefficient and sometimes inaccurate build process Nov 6, 2020
@rly
Copy link
Contributor Author

rly commented Nov 6, 2020

There is one related condition that I want to improve:
Container B contains a link to Container A.
Container A is built.
During the build of Container B, Container A is rebuilt (there is no difference in the end result).

@rly
Copy link
Contributor Author

rly commented Nov 6, 2020

Done. @oruebel @ajtritt please review. This is a relatively bulky PR. Happy to discuss over video.

@rly rly merged commit 1cf4f7c into dev Nov 7, 2020
@rly rly deleted the fix/build_modified branch November 7, 2020 00:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants