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

should extract for RheoFreqData take a FreqDataType #23

Closed
moustachio-belvedere opened this issue Jul 21, 2019 · 5 comments
Closed

should extract for RheoFreqData take a FreqDataType #23

moustachio-belvedere opened this issue Jul 21, 2019 · 5 comments
Assignees
Labels
enhancement New feature or request
Milestone

Comments

@moustachio-belvedere
Copy link
Member

moustachio-belvedere commented Jul 21, 2019

At the moment, extract for RheoTimeData types needs a TimeDataType argument which makes sense because you might want to extract time, or time and stress, or time and strain.

However, extract for RheoFreqData types always just extracts the frequency vector as it doesn't make any sense to ever have Gp without Gpp and vice versa.

So when using extract for RheoFreqData, the second argument of FreqDataType is completely redundant.

I can think of a few options:

  • We keep behavior as it is

  • Remove second argument for RheoFreqData types

  • Rename the RheoFreqData method extractfrequency or something more specialised so it is clear from the function name what it does, and remove the redundant second argument.

@akabla
Copy link
Member

akabla commented Jul 21, 2019 via email

@moustachio-belvedere moustachio-belvedere self-assigned this Sep 5, 2019
@moustachio-belvedere moustachio-belvedere removed their assignment Sep 24, 2019
@akabla akabla self-assigned this May 23, 2020
@akabla akabla added this to the 1.0.0 milestone May 23, 2020
@akabla akabla added the enhancement New feature or request label May 23, 2020
@akabla
Copy link
Member

akabla commented May 23, 2020

I'm wondering if it would not be simpler to rename all extract functions for consistency:

extracttime
extractstrain
extractstress
extractfreq

I think it would make the code cleaner and simpler to read for the user, and for us. Most of the code of the extract function is spent working out which parameter to extract.

Note: these functions are currently defined in processing.jl

@moustachio-belvedere
Copy link
Member Author

Sounds good to me!

@akabla akabla modified the milestones: 1.0.0, 0.9.5 Aug 18, 2021
akabla added a commit that referenced this issue Aug 19, 2021
@akabla
Copy link
Member

akabla commented Aug 19, 2021

The functions extract are still there to avoid breaking changes, but now deprecated. To be removed with version 1.0.

@akabla akabla closed this as completed Aug 19, 2021
akabla added a commit that referenced this issue Aug 19, 2021
Extract deprecated in favour of onlytime/strain/stress/freq #23
@akabla
Copy link
Member

akabla commented Aug 22, 2021

Need to include new functions in API

akabla added a commit that referenced this issue Aug 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants