formatting + code structure comments #1

New Issue

hartytp · 2023-12-24T20:20:55+08:00

hartytp commented

2023-12-24 20:20:55 +08:00

Some of the code here is a bit hard to read (e.g. very long lines). Could you set up a standard python formatter / linter for the code (e.g. flake8+black)?

hartytp commented

2023-12-24 20:24:02 +08:00

Might also be worth adding jupytext to avoid committing binary data.

hartytp commented

2023-12-24 20:25:57 +08:00

When you have code like this

self.period_err, self.phase_err, self.helper_adpll, self.main_adpll, self.gtx_beating, self.main_beating, self.gtx, self.helper, self.main, self.helperfreq, self.mainfreq = simulation_jit(

It's worth thinking about using a datastructure. This is pretty hard to read and easy to make mistakes

When you have code like this ```python self.period_err, self.phase_err, self.helper_adpll, self.main_adpll, self.gtx_beating, self.main_beating, self.gtx, self.helper, self.main, self.helperfreq, self.mainfreq = simulation_jit( ``` It's worth thinking about using a datastructure. This is pretty hard to read and easy to make mistakes

hartytp changed title from ~~formatting~~ to formatting + code structure comments

2023-12-24 20:27:23 +08:00

hartytp commented

2023-12-24 20:29:09 +08:00

I also wonder whether it's really a good idea for WRPLLSim to have a separate run method outside of the constructor. Are there any cases where one doesn't immediately want to call run after construction? Or, where one wants to call run multiple times on the same object?

This almost feels more like it should be a data class, with run -> __post_init__ so it's explicitly part of construction of the object and post construction the object just has the simulation outputs. I also wonder whether the sim code should be inside this object rather than this object wrapping it. The additional indirection just makes the code harder to read without adding any value AFAICT.

I also wonder whether it's really a good idea for `WRPLLSim` to have a separate `run` method outside of the constructor. Are there any cases where one doesn't immediately want to call `run` after construction? Or, where one wants to call `run` multiple times on the same object? This almost feels more like it should be a data class, with `run` -> `__post_init__` so it's explicitly part of construction of the object and post construction the object just has the simulation outputs. I also wonder whether the `sim` code should be inside this object rather than this object wrapping it. The additional indirection just makes the code harder to read without adding any value AFAICT.

Sign in to join this conversation.

No Label

No Milestone

No project

No Assignees

1 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/wrpll-simulation#1