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

small change in the documentation #1957

Merged
merged 1 commit into from
Oct 16, 2024
Merged

small change in the documentation #1957

merged 1 commit into from
Oct 16, 2024

Conversation

FilipeFcp
Copy link
Contributor

Hi @momchil-flex . Please let me know what you think when you have some time:

image

#1949

Copy link
Collaborator

@momchil-flex momchil-flex left a comment

Choose a reason for hiding this comment

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

Sounds great! Maybe also update the FAQ section a little?

@momchil-flex
Copy link
Collaborator

Actually, are you sure about the distinction from PlaneWave? Now that I think about it I'm a little confused about this, because the auxiliary simulation to the TFSF one just uses a PlaneWave. I thought that the difference is only in the area factor.

@FilipeFcp
Copy link
Contributor Author

Hi Momchil. Yes, I understand that the difference is the area factor. A PlaneWave (infinity with periodic boundary) will inject 1 W of power. A TFSF will inject 1 $\frac{W}{\mu^2}$ intensity, or 1/sourceArea power.

Regarding the FAQ, it is describing a num_freq attribute for the SourceTime object, that seems deprecated. Is the normalization still an issue for frequencies distant from the central one? Can I remove the reference for the num_freq attribute?

@momchil-flex
Copy link
Collaborator

Hi Momchil. Yes, I understand that the difference is the area factor. A PlaneWave (infinity with periodic boundary) will inject 1 W of power. A TFSF will inject 1 W μ 2 intensity, or 1/sourceArea power.

What I was questioning was the statement about angled incidence. Is that really true? I thought that the normalization should not depend on the angle and the relationship between TFSF and PlaneWave should be the same at all angles.

Regarding the FAQ, it is describing a num_freq attribute for the SourceTime object, that seems deprecated. Is the normalization still an issue for frequencies distant from the central one? Can I remove the reference for the num_freq attribute?

I think this should be just Source.num_freqs instead of SourceTime.num_freqs. This argument is only available in ModeSource and Gaussian-type sources.

@FilipeFcp
Copy link
Contributor Author

I think you are right. I just tested it and this is what I got for a plane wave:

deg flux
10 1.00007653,
20 1.00046873,
30 1.00205553
40 1.02323759
50 1.07655406

I believe the error for 40 and 50 is due to the fields taking too long to decay.

So I will change this part as well, and open another PR for the FAQ.

@momchil-flex
Copy link
Collaborator

Ah yeah I see, yeah PML are not great at absorbing oblique angles, you could try changing the normal direction boundaries to Absorber and put like 100 layers, might look better.

@FilipeFcp
Copy link
Contributor Author

Hi @momchil-flex,

I ran some tests to fully understand this.

Yes, both the TFSF and Plane wave sources keep their quantities constant in the propagation direction (TFSF intensity and PW power). That information might be outdated, so I believe it's safe to remove it from the documentation and FAQ?

image

By the way, the absorber with many layers worked well for large incidence angles.

@momchil-flex
Copy link
Collaborator

Not sure what you mean? To me that information sounds correct still. The power is constant for a plane that is normal to the source injection axis (the axis normal to the source geometry) right? Meaning that the power in the propagation direction (the angled axis) would have an extra factor of 1 / cos(theta)?

@FilipeFcp
Copy link
Contributor Author

Yes, this is correct. What I meant is that the same applies to both TFSF and PW. The TFSF docstring implies it would be different for a PW. I thought the FAQ had the same issue, but it's actually correct there.

@momchil-flex
Copy link
Collaborator

@FilipeFcp sorry is this ready for merge then?

@FilipeFcp
Copy link
Contributor Author

Hi @momchil-flex. I believe it is.

@momchil-flex momchil-flex merged commit e5699ba into develop Oct 16, 2024
15 checks passed
@momchil-flex momchil-flex deleted the filipe/tfsf_doc branch October 16, 2024 14:05
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.

2 participants