clear bug in bundle algorithm

[pavlis]

Working on the notebooks for the upcoming short course I uncovered an unambiguous bug in how we are handling three component data save and read. I'd handle it but I need a little help to fix it.

The issue is that we are losing the transformation matrix. I am encountering this problem when this little block of code is executed:

from mspasspy.algorithms.bundle import bundle_seed_data
ensemble=TimeSeriesEnsemble()
cursor=db.wf_TimeSeries.find({})
for doc in cursor:
    d=db.read_data(doc,normalize=normalize_list)
    ensemble.member.append(d)
d3c=bundle_seed_data(ensemble)
db.save_ensemble_data(d3c,data_tag='rawdata')

The function bundle_seed_data does what I think it should do and sets the transformation matrix internally. The problem is in save_ensemble_data, which is mostly a loop over the members calling save_data for each. When I look at the code I do not see it handling the transformation matrix. Recall we put this attribute in the schema to handle this:

      tmatrix:
        type: list
        concept: Three-component data's transformation matrix
        constraint: required

where the lists is to be the 9 components of the transformation matrix. Side issue: the yaml file should specify the order the components should appear.

We definitely aren't handling this right. I find nothing in the database.py related to this issue. This is definitely a problem we need to fix immediately as it pretty much invalidates any Seismogram processing.

Some elements of what we have to fix:

• There are two other wired attributes in Seismogram we also need to handle: bool components_are_orthogonal and bool components_are_cardinal. I don't think they are being handled by the writer at all. They also need a definition in the yaml file the writer clearly needs to save these.
• I am pretty sure the Metadata CoreSeismogram constructor has code for handling tmatrix stored as a python list and the equivalent for Seismogram just uses it. (I think) Hence, i don't think the problem is in the reader but the writer. The simple solution, I think, is to have Database.save_data needs a special block to handle Seismogram data. Maybe you have a better idea though?
• I looked at the C code and the constructors will handle the two booleans ok but not as cleanly as they should. There is a comment in the CoreSeismogram that admits this is wrong but the cost is small, which is true. I think fixing the blemish in the C++ code on this issue is minor, but I 100% think the database needs to have these attributes stored. If we get that decided it will be easy to add the logic to the C++ code to check for the existence of the attributes for defining components_are_orthogonal and components_are_cardinal and using the existing logic only if they aren't set.

[wangyinz]

I think the problem is because that the bundle_seed_data is C++ function, so it does not handle the connection from the tmatix dict entry to the transformation_matrix object property. That make sense, but the problem is in the save methods, we need to sync the dict entry with the object property like what we did for time related properties here:

mspass/python/mspasspy/db/database.py

Lines 2162 to 2187 in eda52f2

	def _sync_metadata_before_update(mspass_object):
	"""
	MsPASS data objects are designed to cleanly handle what we call relative
	and UTC time. This small helper function assures the Metadata of
	mspass_object are consistent with the internal contents. That
	involves posting some special attributes seen below to handle this issue.
	Since Metadata is volatile we need to be sure these are consistent or
	timing can be destroyed on data.
	"""
	# this adds a small overhead but it guarantees Metadata and internal t0
	# values are consistent. Shouldn't happen unless the user messes with them
	# incorrectly, but this safety is prudent to reduce the odds of mysterious
	# timing errors in data
	t0 = mspass_object.t0
	mspass_object.set_t0(t0)
	# This will need to be modified if we ever expand time types beyond two
	if mspass_object.time_is_relative():
	if mspass_object.shifted():
	mspass_object['startime_shift'] = mspass_object.time_reference()
	mspass_object['utc_convertible'] = True
	else:
	mspass_object['utc_convertible'] = False
	mspass_object['time_standard'] = 'Relative'
	else:
	mspass_object['utc_convertible'] = True
	mspass_object['time_standard'] = 'UTC'

Note that the reader doesn't have the problem because we explicitly construct the tmatrix from dict in the constructor at this line:

mspass/cxx/src/lib/seismic/CoreSeismogram.cc

Line 121 in 99a8ac3

this->set_transformation_matrix(boost::any_cast<py::object>(this->get_any("tmatrix")));

Anyway, @haruming could you please fix it? Thank you!

[pavlis]

Sounds like a solution. @haruming please let me know when you have this fixed. This breaks a section of the notebooks I'm writing for the upcoming short course at the sage/gage workshop in August.

For the record, I would like to state that from a design perspective I think this is something the C++ code should have to deal with only during construction. The Seismogram object considers a transformation matrix fundamental to defining what it is. We have done this right, I think, in designing a constructor that can build a valid Seismogram object from Metadata created from a MongoDB document. The title of this discussion page is actually wrong because I misunderstood the problem when I began the page. The problem was never in the bundle_seed_data algorithm. It was just the function that broke in my notebook when I tried to read data that had been clobbered by a save_data operation that did not properly save the tmatrix attribute.

A related problem that happens in bundle_seed_data is another constructor for CoreSeismogram. It assembles a CoreSeismogram from three CoreTimeSeries objects with this constructor:

CoreSeismogram::CoreSeismogram(const vector<CoreTimeSeries>& ts,
                       const unsigned int component_to_clone)

That constructor tends to normally leave debris in the Metadata because it copies the Metadata from component_to_clone. An example is chan which is concept mismatch with a Seismogram. I would point out that our clean method in Database can handle this nicely if you just tell it to delete undefined attributes in the schema for wf_Seismogram.

Finally, there is another detail in there that we do also need to resolve and now is the time to do it instead of later. The two booleans, components_are_cardinal and components_are_orthogonal, should be handled explicitly in the database. If you, @haruming , fix save_data and add attributes to the yaml file for these attributes, I will modify the C++ code to use them in the constructors. A detail that got lost above is that when I looked at the code to understand this problem I realized there was a kludgy solution for handling these two attributes in the Metadata constructor. I'll do it right if you can fix the save_data method of Database to save these attributes.

[pavlis]

I just noticed another problem that should be fixed @haruming while you are adding the two boolean attributes to the schema for wf_Seismogram. The current contents are:

  wf_Seismogram:
    data_type: Seismogram
    schema:
      <<: *wf_TimeSeries
      tmatrix:
        type: list
        concept: Three-component data's transformation matrix
        constraint: required
      channel_id:
        type: list
        reference: channel
        constraint: xref_key

That is alright IF the list type is enforced. Note that for wf_TimeSeries the same attribute name is not a list but a single ObjectID. I actually think it is really bad form to have the same attribute name used for data with different types.

Furthermore, the more important issue is that the recommended normalization collection for wf_Seismogram is site not channel. The list of channels can, for example, be misleading if any coordinate transformation is done. I think we can keep a list of channel_id values as an option but it must have a different name and we need to provide for site_id. In short, I advise we change the yaml file section above to this (including proposed names for the two booleans:

      chanid_list:
        type: list
        concept:  Optional list of original channel attributes from with this object was constructed.
        reference: channel
        constraint: xref_key
     site_id:
      type: ObjectID
      concept:  Linking id to site collection
      reference:  site
      constraint:  xref_key
    cardinal:
      type:  bool
      concept:   When true the data stored are orthogonal and in cardinal (ENZ) geographic directions.
      reference:  site
      constraint:  required
    orthogonal:
      type:  bool
      concept:   When true the transformation matrix is orthogonal (inverse can then be constructed from transpose instead of inversion). 
      reference:  site
      constraint:  required

[haruming]

This is an interesting bug and glad we have a solution right now. But two things I am confused about are:

1. in the yaml file section, both cardinal and orthogonal have the reference attribute -> 'site', but the site collection in the yaml file doesn't include these two boolean attributes. Should we have those in the site collection?
2. Correct me if I am wrong, we should also set both the cardinal and orthogonal attributes in the metadata when we are saving or updating the mspass_object, just like what we would do for the matrix.

BTW, I would probably finish fixing this bug by Friday evening.

[pavlis]

On Jul 2, 2021, at 12:36 AM, Will Yang ***@***.******@***.***>> wrote: This message was sent from a non-IU address. Please exercise caution when clicking links or opening attachments from external sources. This is an interesting bug and glad we have a solution right now. But two things I am confused about are: 1. in the yaml file section, both cardinal and orthogonal have the reference attribute -> 'site', but the site collection in the yaml file doesn't include these two boolean attributes. Should we have those in the site collection? That is an error in what I put in the yaml file if i has that. Both belong only in wf_Seismogram. My mistake 1. Correct me if I am wrong, we should also set both the cardinal and orthogonal attributes in the metadata when we are saving or updating the mspass_object, just like what we would do for the matrix. Yes, set cardinal and orthogonal when the document is loaded into Metadata. When that exists let me fix the Metadata constructor in the C++ code to use them. My recollection is the read_data method calls the Metadata constructor to create the object. I will double check that when you get this implemented so treat that as my problem to solve. BTW, I would probably finish fixing this bug by Friday evening. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#216 (reply in thread)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AABNEJZ7CBVIJJP37SEHZOTTVU65XANCNFSM47TDTHXQ>.

[wangyinz]

just a note here. The constructor will set those two variables:

mspass/cxx/src/lib/seismic/CoreSeismogram.cc

Lines 123 to 126 in 82d6582

	if(components_are_cardinal)
	components_are_orthogonal=true;
	else
	components_are_orthogonal=false; //May be wrong but cost is tiny

If you want to set them in the reader, it should overwrite the ones set by constructor.

[wangyinz]

@pavlis Weiming noted an issue when implementing above. The cardinal and orthogonal methods as well as corresponding private variables are not bound to Python right now. That became an issue as we need that piece of info to sync the metadata during save. My initial thought is to just bind the two public methods and not link those variables to metadata. (e.g., d52a4e4). Then, the cardinal and orthogonal keys in metadata are completely disconnected from the private variables on the C++ side. The alternative will be to handle them like npts where we update the metadata entry in the setter. Note that this way the variable and the metadata entry is still half disconnected, because changing the metadata won't do anything to the actual variable. Since they are disconnected anyway, I think the best solution here is to implement a sync method that sync all the metadata entries with their underlying variables. What do you think?