Block API refactoring and implement asdf_ndarray_copy - #245
Merged
Conversation
If the byte order is big-endian we have to specify that by writing the datatype as a mapping, not a bare scalar string datatype.
Still not happy with asdf_block_create--I think it would be more useful if it takes an optional data array at the same time as shortcut; otherwise it doesn't feel like it does much on its own. I was also reminded that emit_blocks_prepare does an awful lot of copying that seems avoidable; it's not going to be nice for large arrays. I need to rethink that one; probably move more of its logic directly into asdf_block_info_write.
embray
added
enhancement
Adds a new public API header asdf/block.h for the block-related functions, separating it better from file.h, and moved most of the block implementation into block.c which was previously rather light. This also cleans up asdf_block_info_write so that *it* manages where the block data is read from, rather than emit_blocks_prepare (now deleted). This allows it to manage from where the block data is read to avoid needless memory allocations and copying.
Still add asdf_file_block_create for internal use when creating blocks that will be associated with a file.
This also allows removing the entire asdf_ndarray_data_alloc_temp kludge, as far as I can tell.
having sat on it a few days I realized that would be a better name for this feature.
Serializing an ndarray that owns its own data (i.e. via asdf_ndarray_data_alloc) now hands that data to the file and its lifecycle, which frees it on write/close, mirroring how block storage already transfers via asdf_block_append. Extensions can embed a data-allocated ndarray without leaking and without an explicit asdf_ndarray_data_dealloc; a redundant dealloc is now a safe no-op. This is much cleaner for extensions that need to create and serialize an ndarray as part of the extension object's serialization.
With the block API and ndarray data management refactoring it's now a bit more straightforward to correctly implement a basic copy method for `asdf_ndarray_copy` (which was previously effectively broken). After careful consideration the decision is taken, for the basic asdf_ndarray_copy, that it would make most sense if it is a deep copy through-and-through, including the data. If the data comes from a binary block, that block is also copied (a new block is appended to the file, whether the ndarray is copied into the source file of the original ndarray, or into a new/different file). Later we will want to add other, more advanced means of creating and copying ndarray data, e.g. allowing an ndarray to share data from an existing binary block, but that is out of scope for the basic implementation.
Didn't want to fully document the asdf/block.h header yet, but went ahead and added a page for it so that references to those APIs from elsewhere in the docs can be resolved. Still left it documented as low-level and somewhat provisional...
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The main purpose of this PR is to resolve #242 and implement a working copy of
asdf_ndarray_copy, which previously was a totally broken foot-gun. I thought of delaying this for an 0.2 release but since 0.1 is still in release-candidate I think it would be good to go ahead and slip this into 0.1 since it has some significant API changes.While starting work on this I had an initial hacky attempt, but wasn't satisfied with it. It occurred to me that we had entirely separate code for working with binary blocks versus working with ndarrays and their data (often but not always backed by binary blocks). There was an unfortunate amount of duplication here, and not really a clean relationship between ndarrays and their underlying blocks.
Here we expand the API for working directly with binary blocks (
asdf_block_t) as a first-class data structure with appropriate abstractions around it (including the ability to create and assign data to a block independently of how it will be used in an ASDF file). Blocks can then be later appended to a file. This adds a few new previously missing block routines as well, such asasdf_block_allocated_size_set.ndarray data handling is then refactored to work entirely through the lower-level block APIs. ndarray data can be held either in a "logical" block or a "physical" block. The internal block representation does have any explicit distinction between "logical" or "physical". It just depends--in the ndarray code--whether the data was inline in the YAML, or from a binary block in a file. In the former case it is still deserialized from YAML and managed internally as a "logical" block. Likewise when serializing an ndarray the decision is made, based on its
asdf_ndarray_storage_t, whether to serialize the block data inline in YAML, or to append it as a new physical binary block in the file.With that refactoring done--ndarray data managed through the
asdf_block_tdata structure--it was also easier then to implement a deep-copy routine that also copies ndarray data between files (in the form of logical blocks).AI Disclosure
No AI tools used.