Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds deep copy functionality to the CAS (Common Analysis Structure) object, allowing users to create independent copies of CAS instances with or without duplicating the associated typesystem.
Key changes:
- Added a
deep_copy()method to theCasclass with optional typesystem copying - Implemented comprehensive test coverage for both copying modes (with and without typesystem)
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| cassis/cas.py | Implements the deep_copy() method to create deep copies of CAS objects, handling sofas, views, feature structures, and their references |
| tests/test_cas.py | Adds two test cases verifying deep copy functionality with and without typesystem copying |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@dmnc-grdnr Could you please consider the Copilot review. If you believe and of its review remarks are wrong, please comment them. |
|
Oh, and thank you again for another PR :) |
|
I reviewed the Copilot suggestions agreed with most, but found that a few lines are actually obsolete and should be removed instead of modified. This is my first pull request and I am unsure how to make the changes without breaking things. Could you please point me to what to do next? |
reckart
left a comment
reckart
left a comment
There was a problem hiding this comment.
Thank you very much. To continue, it would be great if you could simply address the comments by updating the code and then committing the changes into this branch. Once the changes are in, the review will be repeated. Please do not use the "amend" function of git, just add another commit. Avoid force pushes, then you can't really break anything. Thanks!
|
I was finally able to iron out all of the issues that appeared after including the random CAS tests. They were extremely helpful! I think, I made all the changes that you and Copilot required. Could you please have another look? Thanks! |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@dmnc-grdnr coming back to this, I re-ran copilot just for good measure before I would review the code again and possible merge it. Unfortunately, I have to say it found a few points that appear to be quite critical and which I may have quite easily missed in a manual review. Do you still have the capacity and interest to address those issues? |
|
Hello @reckart, it took me some time, but I have not lost interest :) Don't get me wrong, I don't mind applying these safety measuers at all. It just seems that they might be superflous. Then again my analysis might superficial instead ;) (I'd be happy to have call about it too, if you like) |
|
@dmnc-grdnr Best write me a mail. |
|
@reckart That's fine. But I have already posted my questions in my previous comment. |
* main: dkpro#328 - Remove annotations in a specified range dkpro#328 - Remove annotations in a specified range dkpro#328 - Remove annotations in a specified range dkpro#328 - Remove annotations in a specified range dkpro#328 - Remove annotations in a specified range dkpro#328 - Remove annotations in a specified range dkpro#328 - Remove annotations in a specified range dkpro#328 - Remove annotations in a specified range Issue dkpro#342: Improve cas_to_comparable_text dkpro#328 - Remove annotations in a specified range dkpro#328 - Remove annotations in a specified range % Conflicts: % tests/test_cas.py
- ensure original and copy are fully decoupled - Preserve None entries in FSArray copies and avoid warnings for None non-primitive features. - Avoid creating a document annotation when checking for the language of the source CAS - Support FSList and FSArray in cloning
- properly handle collection features that have None value
reckart
force-pushed
the
feature/330-enable-deep-copy
branch
from
21989c9 to
3dca51e
Compare
- Avoid shared collection objects between original and clone
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Fix handling of primitive lists
- Fix handling of inlined vs. reusable arrays
- Fix handling of inlined vs. reusable primitive arrays
- Added more testing
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Fix handling of inlined vs. reusable primitive arrays
- Fix remembering which structures should be in the index and in which index and which should not be indexed
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Remove print calls from tests
- Better tracking of which FS belongs to which view(s)
- No need to pass language and mime-type at CAS construction
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Also properly duplicate sofaArray if set
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
- Preserve active view
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| for item in fs.elements: | ||
| if item is None: | ||
| referenced_list.append(None) | ||
| elif hasattr(item, "xmiID") and item.xmiID is not None: | ||
| referenced_list.append(item.xmiID) | ||
| else: | ||
| warnings.warn( | ||
| f"Standalone FSArray {fs.xmiID} contains an unidentifiable item; preserving as None in copy." | ||
| ) | ||
| referenced_list.append(None) |
| referenced_list.append(None) | ||
| elif hasattr(item, "xmiID") and item.xmiID is not None: | ||
| referenced_list.append(item.xmiID) | ||
| else: | ||
| warnings.warn( | ||
| f"Array feature '{feature.name}' of FS {fs.xmiID} contains an unidentifiable item; preserving as None in copy." | ||
| ) | ||
| referenced_list.append(None) |
| for item in fs.elements: | ||
| if item is None: | ||
| referenced_list.append(None) | ||
| elif hasattr(item, "xmiID") and item.xmiID is not None: | ||
| referenced_list.append(item.xmiID) | ||
| else: | ||
| warnings.warn( | ||
| f"Standalone FSArray {fs.xmiID} contains an unidentifiable item; preserving as None in copy." | ||
| ) | ||
| referenced_list.append(None) |
| head = current.head | ||
| if head is None: | ||
| referenced_list.append(None) | ||
| elif hasattr(head, "xmiID") and head.xmiID is not None: | ||
| referenced_list.append(head.xmiID) | ||
| else: | ||
| warnings.warn("FSList item without xmiID encountered during deep copy; preserving as None in copy.") | ||
| referenced_list.append(None) |
This PR adds the deep copy functionality to the CAS object.
Tests for the function was added to test_cas.py
I have verified the functionality on different CAS files, but due to the complexity of the data structure, there might be something I have overlooked.
Therefore feedback is highly appreciated!