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

WFS 2.5D point source #117

Merged
merged 1 commit into from
Mar 15, 2019
Merged

WFS 2.5D point source #117

merged 1 commit into from
Mar 15, 2019

Conversation

fs446
Copy link
Member

@fs446 fs446 commented Mar 11, 2019

I remember we had this discussion a while ago in the Matlab toolbox. I try it here again :-)
I'd vote to change our default WFS 2.5D point source to that driving function originating from Delft, which is compatible with the recent works of Firtha, i.e. the unified WFS framework.
Our currently used version is -- due to the further approximation -- not easy to handle in terms of

  • consistent and proper amplitude normalization
  • introducing/defining other referencing schemes

The PR is merge prepared in terms of our latest changes w.r.t. driving function and docstring example handling, but not for literature references.

I would not mind to supply both driving functions with a consistent and proper name scheme. I did not come up with a meaningful idea, though.

@fs446 fs446 requested review from spors, mgeier and hagenw March 11, 2019 14:49
Copy link
Member

@hagenw hagenw left a comment

Choose a reason for hiding this comment

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

The actual implementation of the equation looks good, I was not able to spot an error.

As I was no longer involved in actual WFS theory for some time now, I totally trust you with the decision that this is the better default driving function ;)

sfs/mono/drivingfunction.py Outdated Show resolved Hide resolved
sfs/mono/drivingfunction.py Outdated Show resolved Hide resolved
sfs/mono/drivingfunction.py Outdated Show resolved Hide resolved
sfs/mono/drivingfunction.py Outdated Show resolved Hide resolved
sfs/mono/drivingfunction.py Outdated Show resolved Hide resolved
@fs446
Copy link
Member Author

fs446 commented Mar 14, 2019

The actual implementation of the equation looks good, I was not able to spot an error.

As I was no longer involved in actual WFS theory for some time now, I totally trust you with the decision that this is the better default driving function ;)

Thanks!
Don't wait for this issue for the next larger release. I'm not sure if/when to handle this within the next days.

@fs446
Copy link
Member Author

fs446 commented Mar 14, 2019

I made a new push including a master rebase, so this version is up to date with latest dev.
I've added some more comments on the 3D/2D Rayleigh integral handling to give the reader more hints what's going on. I'd vote to leave the cited literature in the docstring for now to give users/readers the chance for fast access of the change.
I've introduced 'point_25d_legacy' that implements the point_25d of versions <=0.4.0 . This could get another name though. I've thought about something like
def _point_25d_from_3d()
def _point_25d_from_2d() (the legacy)
and having point_25d = _point_25d_from_3d

I also introduced a normalising gain in both the examples (which is open for all other examples as well) to make more clear that legacy is not amplitude correct at ref point, but the new one is.

@fs446
Copy link
Member Author

fs446 commented Mar 14, 2019

I've thought about something like
def _point_25d_from_3d()
def _point_25d_from_2d() (the legacy)
and having point_25d = _point_25d_from_3d

I'm not sure how to handle the docstring docu then.

@mgeier
Copy link
Member

mgeier commented Mar 15, 2019

I like the name point_25d_legacy().

@fs446 fs446 requested a review from hagenw March 15, 2019 13:44
@mgeier
Copy link
Member

mgeier commented Mar 15, 2019

You should define the "legacy" function after the new function.

sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
sfs/mono/wfs.py Outdated Show resolved Hide resolved
@fs446
Copy link
Member Author

fs446 commented Mar 15, 2019 via email

@mgeier
Copy link
Member

mgeier commented Mar 15, 2019

but not doing the same principle for the referencing part

Indeed.

We could, instead of using BibTeX, manually add plain Sphinx citations (https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#citations) to the docstrings.

Then your concern would be satisfied.

I just don't think it's worth it.

The bibtex entry tag alone does not hold this information, I hope we agree on this.

We do.

I could imagine that parsing a DOI out of docstring and grabbing all information for creating a reference list is something people ask for or even have done.

I don't know if anyone has asked for it, but I'm quite sure nobody has implemented it.

The current framework is very good, but we should always try to improve.

Sure, there is room for improvement in the current situation.
But I think the BibTeX extension we are using is state-of-the-art, I didn't find any better extensions.

-old handling was Spors/Rabenstein, 2D to 2.5D
-new one is Delft, 3D to 2.5D
@fs446
Copy link
Member Author

fs446 commented Mar 15, 2019

But I think the BibTeX extension we are using is state-of-the-art, I didn't find any better extensions.

Good job on that. I will remind myself from time to time to have a look on this issue. Can be closed for the moment.

@fs446 fs446 merged commit c74e34a into master Mar 15, 2019
@fs446 fs446 deleted the wfs_25d_point branch March 15, 2019 21:31
@fs446
Copy link
Member Author

fs446 commented Mar 15, 2019

thx for your support!!

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.

3 participants