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

Impr/fix refactor sfcf read #164

Merged
merged 19 commits into from Mar 15, 2023
Merged

Conversation

jkuhl-uni
Copy link
Collaborator

This should fix #163. I refactored large parts of the read_sfcf method and I am now using the same method to sort the replika as the openQCD methods do since #153.
Although I found a small mistake in my analysis script that could also be the reason #163 appeared in the first place, like this, the sorting of replika should be more consistent between the read methods of sfcf and openQCD data.

@jkuhl-uni jkuhl-uni requested a review from fjosw as a code owner March 15, 2023 08:36
@fjosw
Copy link
Owner

fjosw commented Mar 15, 2023

Thanks for the fix. Some of the tests seem to be failing. I think the reason could be that start_read is only set when a pattern is found and does not have a value in other cases.

Copy link
Owner

@fjosw fjosw left a comment

Choose a reason for hiding this comment

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

I started looking into your changes in sfcf.py and saw a few lines of duplicate code:

  • I think lines 85 & 86 can be removed as they are overwritten in the next block anyways
  • Either lines 74 & 75 or 103 & 104 could be removed.

Another thing that I haven't been very consistent with is raising more specific Exceptions (for example in _find_correlator).

@jkuhl-uni
Copy link
Collaborator Author

Oh yes, oyou are right... I have to say I did not have a look into the kwargs statements, thank you for telling me.
About the Exceptions that you are mentioning:
Do you mean I should alter the description or the Exception type?
If the second one is the case, what do you have in mind (without writing a custom exception class, that is)?

@jkuhl-uni
Copy link
Collaborator Author

Thanks for the fix. Some of the tests seem to be failing. I think the reason could be that start_read is only set when a pattern is found and does not have a value in other cases.

This is in fact the case... the only solution I would have is to throw an exception in this case... However interestingly, in my test set-up this does not appear... I am using Python 3.10.6 with pytest 7.2.2. As I cannot reproduce the error, I cannot fix the function (or the test for that matter).

@fjosw
Copy link
Owner

fjosw commented Mar 15, 2023

In case of _find_correlator we could maybe got with ValueError?

One way of fixing the error could be to add an else statement at the end of the for loop (line 380) which gets only triggered when the loop does not break. This case could result in an exception. In case that does not solve the issue I could have a closer look myself.

@fjosw
Copy link
Owner

fjosw commented Mar 15, 2023

I also cannot reproduce the error on my machine. Maybe the error is connected to the way how you clean the test environment. I would suggest using pytest's tmp_path feature which takes care of this automatically. I used it for example in pandas_test.py.

@fjosw
Copy link
Owner

fjosw commented Mar 15, 2023

Okay, this brings us one step closer. Apparently the tests fail because the program tries to extract the correlator f_1 from file tests/data/sfcf_test/data_a/data_a_r0.F_V0. Any idea why?

@jkuhl-uni
Copy link
Collaborator Author

I am already looking into that, but so far I have no idea... the only explanation I can come up with would be, that the exclusion in ll. 211,212 in sfcf.py are not working in this very specific case... Why this would be the case... I don't know...

@fjosw
Copy link
Owner

fjosw commented Mar 15, 2023

I think

ls.sort(key=lambda x: int(re.findall(r'\d+', x)[-1]))

should maybe not be in the for loop?

@jkuhl-uni
Copy link
Collaborator Author

Yep, I just saw that line too... weird that this does appear in any case...
Sorry for spamming commits, but again, I cannot reproduce that for some reason, which makes no sense...

@jkuhl-uni
Copy link
Collaborator Author

That seems to have done the trick. Now I also remember, that a similar issue was also present in a very early version of the code, except that this clipping never appeared in reality back then.

@fjosw
Copy link
Owner

fjosw commented Mar 15, 2023

Thanks a lot. I will merge these changes so that we can do some extended testing on develop.

@fjosw fjosw merged commit 41fec09 into fjosw:develop Mar 15, 2023
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.

Bug coming from difference in search methods in sfcf inputs
2 participants