Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Persist CQT device #19

Merged
merged 3 commits into from
Dec 1, 2023
Merged

Persist CQT device #19

merged 3 commits into from
Dec 1, 2023

Conversation

ben-hayes
Copy link
Contributor

Closes #17

Opted to remove the device parameter from DataProcessor constructor and let device management take place through nn.Module. Sampling rate can now be passed in constructor, and an initial set of CQT kernels are computed. These are updated, with device persisting, whenever sampling rate is changed.

Copy link
Collaborator

@aRI0U aRI0U left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that's cool, apart from this sampling_rate as arg thingy I like your solution

def __init__(self,
step_size: float,
bins_per_semitone: int = 3,
device: torch.device = torch.device("cpu"),
sampling_rate: int = 44100,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it can be done differently, I'd prefer not having to specify a sampling_rate (thus calling _init_cqt_layer) in the __init__ to keep it as lazy as possible. Is this add-on necessary for solving the device issue?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with lazy init is that after construction, neither DataProcessor nor its submodules have any buffers or nn.Parameters registered, so any calls to .to(), .to_empty(), .cpu(), .cuda(), .xpu(), .ipu(), or .apply() will not automatically apply to CQT kernels constructed later. As far as I can see, you have the following choices:

  1. Override all these methods to update self.device. The main downside here is that you will take on the maintenance cost of staying up to date with any future API changes to nn.Module
  2. Override .apply() and store all function pointers on the instance, then apply them in sequence to every new CQT instance that is constructed. Depending on what is passed to apply, this could be quite brittle. e.g. applying any function that relies on external state.
  3. Create a dummy buffer on DataProcessor, and use this to track device changes. This is a bit hacky, but it will let you keep lazy initialisation.
  4. Specify sample rate on construction, immediately registering CQT kernel buffers on the module. IMO this is most idiomatic solution, but of course it comes at the expense of lazy initialisation.

To me, it's not totally clear what is gained by lazy initialisation of the CQT, so 4 makes most sense. But if lazy init is essential then 3 is probably the solution that is least likely to require further maintenance.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll probably go for solution 3, I want my code to be as lazy as me

@aRI0U aRI0U merged commit 8201e4f into SonyCSLParis:dataprocessor-device Dec 1, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CQT kernel device is reset when sample rate is changed
3 participants