SteveDoyle2 / pyNastran

A Python-based interface tool for Nastran's file formats
Other
384 stars 147 forks source link

BDF deep copy is not cross referenced and does not allow to create second deep copy #750

Closed fmamitrotta closed 10 months ago

fmamitrotta commented 10 months ago

Summary

These are actually two distinct issues:

  1. If I create a deep copy of a cross-referenced BDF object, the deep-copied object is not cross-referenced.
  2. If I try to create a deep copy of the deep-copied BDF object, the code fails with the following error:
    Traceback (most recent call last):
    File "c:\Users\qa21944\Github\phd-jupyter-notebooks\notebooks\test.py", line 5, in <module>
    second_deepcopy = first_deepcopy.__deepcopy__({})
    File "c:\users\qa21944\github\pynastran\pyNastran\bdf\bdf.py", line 4768, in __deepcopy__  
    setattr(result, key, deepcopy(value, memo))
    File "C:\Users\qa21944\Anaconda3\envs\phd-notebooks\lib\copy.py", line 161, in deepcopy
    rv = reductor(4)
    File "c:\users\qa21944\github\pynastran\pyNastran\bdf\case_control_deck.py", line 63, in __getstate__
    del state['log']
    KeyError: 'log'

For the second issue I've seen that the problem is in the __getstate__ method of the CaseControlDeck class, when the state dictionary does not have the 'log' key.

    def __getstate__(self):
        # Copy the object's state from self.__dict__ which contains
        # all our instance attributes. Always use the dict.copy()
        # method to avoid modifying the original state.
        state = self.__dict__.copy()
        # Remove the unpicklable entries.
        del state['log']
        return state

This should be easily solvable using state.pop('log', None) in place of del state['log']. I can create a pull request if this is ok.

As far as the first issue is concerned, I actually don't know whether not cross-referencing the deep-copied BDF object is done on purpose. If it's not done on purpose, I would propose to modify the __deepcopy__ method of the BDF class in the following way:

    def __deepcopy__(self, memo: dict[str, Any]):
        """performs a deepcopy"""
        #newone = type(self)()
        #newone.__dict__.update(self.__dict__)
        #return newone
        cls = self.__class__
        result = cls.__new__(cls)
        memo[id(self)] = result
        for key, value in self.__dict__.items():
            setattr(result, key, deepcopy(value, memo))
        if result._xref:  # if original object was cross-referenced, cross-reference the deep copy
            result.cross_reference()
        return result

Also for this I can create a pull request if this solution is acceptable.

Reproduction

Run the following code with the attached bdf file:

from pyNastran.bdf.bdf import read_bdf
from pyNastran.bdf.mesh_utils.mass_properties import mass_properties

original_bdf_object = read_bdf('euler_column_linear_buckling.bdf')
first_deepcopy = original_bdf_object.__deepcopy__({})
mass = mass_properties(first_deepcopy)[0]  # fails because of missing cross-reference, comment to reach next line
second_deepcopy = first_deepcopy.__deepcopy__({})  # fails while deep-copying

euler_column_linear_buckling.zip

Versions

platform.python_version() --> 3.9.16 pyNastran.__version__ --> 1.4.0+dev.no.checksum.error (this commit)

SteveDoyle2 commented 10 months ago

I guess I don't really understand the deepcopy interface that well, but I guess my first question is why do you not have a log? It should be there.

fmamitrotta commented 10 months ago

Good question. Where should the log come from and what does it represent? If you give me an idea of this I can try to understand why I don't have a log.

SteveDoyle2 commented 10 months ago

ahhh...to make pickle/shelve work, getstate (from CaseControlDeck) is used.

    def __getstate__(self):
        # Copy the object's state from self.__dict__ which contains
        # all our instance attributes. Always use the dict.copy()
        # method to avoid modifying the original state.
        state = self.__dict__.copy()
        # Remove the unpicklable entries.
        del state['log']
        return state

Removing that fixes deepcopy, but breaks the other one :/

I think the solution is probably some other double underscore method. I looked at __reduce__, but that doesn't seem to be it. __deepcopy__ is greek to me. I can copy stuff blindly, but that tends to not work

SteveDoyle2 commented 10 months ago

I added your deepcopy code. It was necessary cause getstate was messing it up.

The xref bit is in there too. It's slower and not guaranteed to work, but you can turn it off if you want.