Move away from simpleNamespace to class definitions

[schmidtijoe]

Hello all,
Intro
I am in the development process of a sequence and loved that this repo is here (since i dislike matlab :D) and got a running version of the sequence up for testing on the scanner pretty quickly. However, i encountered some issues when testing the sequence which i couldn't pinpoint too well initially.
I thought part of what made the following debugging hard for me was the use of all objects as types.SimpleNamespace() (SN) since it is not easily trackable (at least with my skillset... , and that was one of the reasons i dislike matlab).
Personally, i think python supports okish type checking mechanisms and i think debugging and developing would be much easier when declaring proper objects.

Idea
I started in my own fork to add a class for gradient, rf, adc and timing to give each their own objects and then refactored methods like make_trapezoid or make_sinc_pulse as individual class members of the object they belong to. I wrote up a quick plot method for each of those classes for quick double checking, which comes in handy when creating objects.
What intrigues me most about the approach is that you get a comprehensive overview of all methods and variables connected to an object.
For example, in my process i was concerned whether or not i can set an adc.phase_offset parameter. I can find it in the make_adc method, but after creation, within the code, as the package is now i have no chance of finding out which variables i can set and which parameter SN objects inherit (i guess that is the convenience in using it) if i want to handle my adc object.
But i think it would be much more beneficial (besides knowing all the methods of the classes) to actually know which parameters define each object and which of those actually are shipped in a meaningful way to the scanner. Since with the SN way of doing things i can easily define adc.random_variable and it will never throw any errors. But i need to follow the object along quite a loong way to find out if the random_variable ever is translated into doing something in my actual measurement. I could also by accident set adc.phaseOffset or whatever typo and it doesnt matter in the process.

Surely, from having to go through this debugging process, i know my way around the package better now and especially with a proper python IDE its not too hard to find what actually is written in the .seq for each object. I just think it would make much more sense to not be dependent on sufficiently naming everything. Eg. i could create an object called adc with my RF class and still it is trackable and known throughout the code that it is only an RF object and can only be used for RF things (i know i am being stupidly annoying now).

Misc
Since my use case was quite limited in the methods i needed, i think the refactoring is not that big of a deal for my project. Obviously going into the meat of the code, where .seq is written etc., porting the code might be bigger of a project (i haven't checked yet).
I was wondering if people would think putting the limited time in (i am researcher myself obviously) would be beneficial for the rest of the users.
Also i think proper definition of which variables entirely defines the objects is subject to debate. For example, I was using trapezoid and extended_trapezoid gradients and think a "compressed" waveform is what easily defines both and from where i can generate a gradient-rastered waveform and easily deduce things like the grad.first, grad.last and grad.shape_dur etc.. A grad.amplitude only makes sense for one of them, hence, i changed it to max_amplitude. In principle also a delay could be deduced via a method from the above but also could just get a variable.

All in all, i guess it would make debugging straight forward and feature development more consise. I am keen to know what people think.

[sravan953]

Hi @schmidtijoe !

I was excited to read through your proposal. I agree, the flexibility of the SimpleNamespace class also leads to confusion and allows users to make avoidable typos. I think your idea of event-specific classes will be worth pursuing, as it will make PyPulseq more Pythonic and make it easier to work with and debug.

I took a look at your fork, and looks like you've refactored all of make_trapezoid, for example, under gradient.py? That would require significant refactoring of all existing example scripts, and also require users to now import Gradient instead of make_trapezoid, for example. My only concern is that this would alienate PyPulseq from Pulseq-converters, even if only slightly.

As an alternative, what if make_trapezoid created a Gradient object on line #155? That way, there are no user-facing changes, but all the debugging benefits that come with event-specific classes. So the user could do: trap = make_trapezoid(params here); and since trap is now a Gradient object, they could use the Gradient-specific plot() that you suggested. Also, the user cannot make typos or try to set irrelevant attributes now.

Keen to hear your thoughts,
Sravan

[schmidtijoe]

Hi @sravan953 . Thanks for taking the time.
My repo is quite a mess at the moment. Since i was obviously facing exactly the problem of defining when or how to interface with the rest of the codebase. Since refactoring everything would be too much surgery and would, as you say, create some compatibility problems.

Initially i liked the idea of refactoring the methods to the objects they belong to. i thought it would make the whole code more intuitive. I also liked that i can handle timing and similar stuff with dedicated event methods (like calc_duration) and not need to find them somewhere else. But i see your comment and surely if you want to keep the file structure one can easily change the corresponding lines in each method and not refactor the methods.

I guess if each class gets a method that "converts" to the original SN at some point (maybe when seq.write is called, or when adding the blocks), it would be a "hybrid" between my original idea and your suggestions. Then the code base doesnt need to be changed and still one would have the benefit of seeing all the variables and get some additional methods like plot or timing stuff when handling the events in the sequence development process.

[schuenke]

Hi @schmidtijoe and @sravan953,

I also really like the idea of introducing classes instead of SimpleNamespaces and would be glad helping in the refactoring/restructuring process. However, I definitely agree with @sravan953 that we should ensure compatibility with existing example scripts and thus with all user-defined sequences being used at the moment.

Thus, I would in general try to stick to the current naming, which is inspired by the Matlab code and enables easy transitions from Matlab to Python. In my opinion, this should have highest priority. A structure similar to the one @sravan953 suggested should do fine.

However, I would suggest to focus on the release of a stable version 1.4 first and start the refactoring afterwards. At this point, I would maybe make sense to introduce some (further) unit tests for the individual functions and methods and make use of GitHub Actions for automatic builds and tests, no?!

Maybe we can schedule a zoom meeting to discuss the topic and collect some ideas. What do you think?

Best,
Patrick

[schmidtijoe]

Hi Patrick,
thanks for the input. After some initial matlab coding i never used the matlab version again, and didnt need to interface between the two. But it sure is a good idea to keep the portability in case new or improved stuff comes from that end.

I was just implementing some ideas (mainly about the structure i envisioned) in my repo and surely most of the example scripts throw errors quite quickly.
I guess with some proper class structure definition one can get those fixed quickly. But if thats not wanted then one has to go on about the refactoring differently. But for that i would think a zoom meeting would be a good start. And personally i dont mind doing this later, after 1.4. Its not a pressing issue.

further ut and automatic builds would be a nice improvement.

Best,
Jochen

[sravan953]

Hi!

Yes, I am completely onboard about PyPulseq 1.4.0 being the first priority. I have been working on it for a few weeks, but haven't made much progress. I will open another discussion, maybe you can help me make progress.

Yes, Github's features can be leveraged to make automate the development cycle

Along with discussing refactoring the SimpleNamespace classes, we could discuss the above points on a call.

Sravan

[sravan953]

Hi all,

I will make a Slack channel so we can coordinate better to figure out when we can meet, would that work?

[schuenke]

Slack is fine for me. Thanks!

[schmidtijoe]

works for me too.

[sravan953]

Hi all! Apologies for the delayed response, please join me on Slack!

[sravan953]

Can you not join by creating an account on the iarg-cmrrc.slack.com workspace?

[schmidtijoe]

I can join some general channels when creating via the link. but it tells me i need to update to see the shared channels and to follow the above invitation.

[sravan953]

@schuenke I apologize for this inconvenience, but I request you to rejoin the channel as a registered member of the iarg-cmrrc.slack.com workspace if possible. That might 'downgrade' the channel to a standard channel.

[schuenke]

@sravan953 I tried to create a new account on your workspace, but I think it's not possible without an IARG e-mail address. I am not really familiar with slack, but I think you have to allow different e-mail suffixes like @gmail.com to join the workspace. However, as far as I understood, I would have access to all public channels in your workspace that way. Probably you don't want this...

Maybe using something else than slack is the way to go? Any ideas?

[sravan953]

Hi @schmidtijoe , thanks for waiting! With @schuenke 's help I realized that you first need to join the workspace, and then channel. Please join the workspace with this invite link -- https://join.slack.com/t/iarg-cmrrc/shared_invite/zt-1mvmdli7a-Huvv3nRcSPl097M2QGdRfw