Remove deprecated setter methods on Frequency

[eendebakpt]

Remove two deprecated setter methods on the Frequency object.

Note: the Frequency object is now mostly immutable except for the unit and a few public methods (e.g. round_to).

[FranzForstmayr]

Do we need the unit to be mutable?

[jhillairet]

Do we need the unit to be mutable?

I think yes. Often the unit is set in Hz and one would like to change to MHz/GHz to get nice plot tick axis labels.

[FranzForstmayr]

Ok, this is the unit for representation. The underlying frequency vector stays the same after change which is fine.

[FranzForstmayr]

We've still the round_to method which writes the f vector. Is this covered by any test? I don't think this should work after this change....

In addition the drop_non_monotonic_increasing method writes self._f. I'm not sure how we should handle this, when the frequency vector should be immutable?

[cafeclimber]

Could it just return a new Frequency with those updated values?

[eendebakpt]

The tests pass because the frequencies are constructed using Frequency.from_f which modifies the _f and reverts the writable flag. If we add the writable flag there, the test for drop_non_monotonic_increasing will fail.

We can modify the drop_non_monotonic_increasing on the Frequency object, but not in a backwards compatible way (currently it modifies the Frequency inplace and returns the list of invalid indices).

The case of round_to is similar: making the Frequency._f read-only will break the round_to.

[eendebakpt]

I will remove the self._f.setflags(write = False) from this PR, it would break existing code without any deprecation warnings.

We could add deprecation warnings to the methods that modify the Frequency (e.g. the .f setter, round_to, drop_non_monotonic_increasing) and provide alternatives that work with an immutable Frequency, but I am not the one to decide on this.

[jhillairet]

We could add deprecation warnings to the methods that modify the Frequency (e.g. the .f setter, round_to, drop_non_monotonic_increasing) and provide alternatives that work with an immutable Frequency, but I am not the one to decide on this.

I think it is a good idea to add a deprecation warning.

Then make Frequency immutable for version 1.0 ?

[eendebakpt]

We could add deprecation warnings to the methods that modify the Frequency (e.g. the .f setter, round_to, drop_non_monotonic_increasing) and provide alternatives that work with an immutable Frequency, but I am not the one to decide on this.

I think it is a good idea to add a deprecation warning.

Then make Frequency immutable for version 1.0 ?

What will we do with the unit? It is not only used in representation methods, but also in computations. For example the Network,__getitem__ or Frequency.overlap_freq. (see comment #878 (comment))
Making the Frequency immutable means making the .unit fixed, or also deprecating methods that use the unit for computations.

We can deprecate the round_to, but should also provide alternatives. The original methods are inplace, so the best option seems to be to provide non-inplace alternatives with a different name, e.g.

def round_to_precision(self, val: Union[str, Number] = 'hz') -> Frequency:

[jhillairet]

Making the Frequency immutable means making the .unit fixed, or also deprecating methods that use the unit for computations.

Good question. Personaly, I often have to change the unit to get nicer print or plot. So I think a way to change the unit is needed, but I'm biased.

[jhillairet]

anyone else's advice? What should we do with this?

[eendebakpt]

anyone else's advice? What should we do with this?

@jhillairet I updated the description of the PR to remove deprecated setter methods. The unit is left unchanged, I think this is best done in another PR (if there is consensus on how to move forward).