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

Fix for filter filename not found or not string #7

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

N0D3J1TQU0
Copy link

No description provided.

general fix to run the example in the readme file
general fix to run the example in readme file
@N0D3J1TQU0 N0D3J1TQU0 changed the title Patch 2 Fix for filter filename not found or not string Aug 18, 2022
@@ -79,7 +79,7 @@ def __init__(self,
global scipy

# check that we were passed a file and it exists
if type(filename) == type(''):
if type(filename) == type(str('')):
Copy link
Owner

Choose a reason for hiding this comment

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

This change doesn't quite make sense to me. What is the value of filename that is causing you trouble? Your proposed change is going to break an important input check.

Copy link
Author

Choose a reason for hiding this comment

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

mmmm in my case if instances were not working because type('') returned "unicode". I just printed type(filename) across all the script and noticed it was always an string instance. So this modification worked for me

Copy link
Author

Choose a reason for hiding this comment

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

This was my problem, even with filters format, directory, and call adress were supposedly correct
ValueError: Please pass the filename for the filter transmission file!

@@ -1025,7 +1025,7 @@ def get_solar_observed_mags(self,
mags = np.zeros((len(zs), len(filters)))

# loop through filters one at a time.
for (i, filter) in enumerate(filters):
for (i, filter) in enumerate([filters]):
Copy link
Owner

Choose a reason for hiding this comment

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

filters should already be a list, so wrapping it in brackets is only going to completely change the meaning of this for loop. What exactly is the problem you are trying to address with this change?

Copy link
Author

Choose a reason for hiding this comment

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

yeah i think this problem was solved with the type(str('')) change. Since if instances were not working, filters reach this line as a string. Since I was too paranoid I just upload all the modifications (i.e. this is not raising an error to me probably because not using that definition or if instance).
Sorry for ambiguity, rookie github user here.

@cmancone
Copy link
Owner

This whole type(str('')) thing doesn't make much sense, so it would really be good to understand what is different in your case to cause this issue. I don't like making changes without understanding why, because that's a good way to break things or to hide the root issue. Try running the following locally vs an online python executor (e.g. online-python.com):

print(type('') == type(str('')))

They are exactly equal, which means that your change doesn't actually change anything.

If for some reason there was a weird edge case where the type of '' was not a string, then I'd just as soon change the check to:

type(filename) == str

Which is a bit more clear anyway. However, I still don't want to do that for no reason, and I don't understand why your proposed change would actually change anything.

@N0D3J1TQU0
Copy link
Author

Yes, they are exactly the same! its probably a problem related to my conda environment, dependencies, etc...
I can't tell exactly what the problem is or which outdated package is generating the problem, but still type(filename) == str (that is indeed more clearly) solved my problem. If it's okey with you this branch should stay open, idk, maybe it will help someone with the same problem or maybe someone will adress what the real problem is.

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