DldAux channel

[zain-sohail]

sed/sed/loader/flash/loader.py

Lines 415 to 424 in 4a8f074

	if channel == "dldAux":
	# Checks the channel dictionary for correct slices and creates a multicolumn DataFrame
	data_frames = (
	Series(
	(np_array[i, value] for i in train_id.index),
	name=key,
	index=train_id,
	).to_frame()
	for key, value in channel_dict["dldAuxChannels"].items()
	)

I was writing some tests/refactoring and looking at raw data and noticed even though we define dldAux as per_pulse, the data is only updated per_train. Am I wrong in that?

If I am right, for improved consistency, I'd recommend we move this logic to create_dataframe_per_train and change our config files to have dldAux in per_train instead of per_pulse as it is now.

[steinnymir]

Yes, you are right. the values seem to be updated every train.
Is the loader actually loading an array with one value and then all nans in this case? If so, it certainly makes sense to make it a per_train channel.

[rettigl]

This is mostly semantics, and I understand why it was labelled per_pulse before (actually it comes from the per-electron tables...). But you are right, they are coming per train, so indeed we should change this. We should, however think about how we communicate these kind of changes that invalidate previous config files. The change requested in issue #173 is of similar nature.

[steinnymir]

These kind of changes which break retrocompatibility should be done only when changing the major version. We can add them once we push a v1.0.0 version maybe?
Also, we might want to keep track of all these and make release notes pointing this out.

[zain-sohail]

Yes, you are right. the values seem to be updated every train. Is the loader actually loading an array with one value and then all nans in this case? If so, it certainly makes sense to make it a per_train channel.

Currently it takes only the number of values that the train has. So if train is 50, it'll take first 50 from dldAux. So that part is fine. As Laurenz said, it's more about semantics. Even though I wrote that code, I had a hard time grasping what was happening, and that's not good.
Conveying changes through release notes when v1.0.0 ships is a good idea. I'd suggest if we have any minor issues, that we merge them first and then merge the ones to main that break compatibility.

[steinnymir]

yes, the loader is some times very hard to read through. A refactoring would be great!

We could consider, for the major version change, to start a V1.0.0 branch where to push non-retrocompatible changes, and that we keep up to date with main. What do you think?

[zain-sohail]

I think that's a good idea.
But I don't know best practices when it comes to release branches. I think it'll be a lot of hassle to manage and fix bugs in multiple branches.

I'd say we merge all the important changes (features and bugs) that are retrocompatible and create a Pypi release v0.2.0 for example. Afterwards we can just continue with structural changes in main. And have the v0.2.0 in a branch in case we have to fix some bug but no new features are added to it.

[zain-sohail]

#330 addresses this with the validator checking if user incorrectly defined and correcting that.

[zain-sohail]

changed to per_train now. Will need to do the check in pydantic later