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 cMethods typo #532

Merged
merged 1 commit into from
Nov 27, 2018
Merged

Fix cMethods typo #532

merged 1 commit into from
Nov 27, 2018

Conversation

Msegade
Copy link
Contributor

@Msegade Msegade commented Nov 7, 2018

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at ?% when pulling a70a838 on Msegade:master into 8672243 on SteveDoyle2:master.

@coveralls
Copy link

coveralls commented Nov 7, 2018

Coverage Status

Coverage remained the same at ?% when pulling a70a838 on Msegade:master into 8672243 on SteveDoyle2:master.

@SteveDoyle2
Copy link
Owner

Why is this a typo?

In bdf/bdf_interface/attributes.py, I see:
self.cMethods = {} # type: Dict[int, Any]

@Msegade
Copy link
Contributor Author

Msegade commented Nov 8, 2018

Because it is written cmethods, not cMethods. Sorry if I'm missing something obvious here.

@SteveDoyle2
Copy link
Owner

Maybe I'm missing something, but why?

If it's going to change, it needs to be changed in all the places where it's used.

@Msegade
Copy link
Contributor Author

Msegade commented Nov 9, 2018

If I define a complex methods in subcase 1 in my BDF file like this:
EIGC,1,HESS,MAX,,12
and I read the BDF using pynastran with:
model = BDF()
model.read_bdf(bdf_filename)
when I try to get the values using the CMethod method:
model.CMethods(1)
I get the error
AttributeError: 'BDF' object has no attribute 'cmethods'
Because as you pointed out, the attribute its defined as 'cMethods'.
So changing the name to 'cMethods' in the 'get_methods.py' file
(it is correct in other parts of the code), solves the issue.

@Msegade
Copy link
Contributor Author

Msegade commented Nov 27, 2018

As I don't understand why you closed this. Can you please explain why this simple test fails? (I'm using one of the .bdfs in the models folder)

test.zip

@SteveDoyle2
Copy link
Owner

I thought you were going to update the pull request to update the name of the variable everywhere instead of just 1 place. That's why I didn't accept it right away.

It sat for 2.5 weeks, so I closed it.

I can look at the model later (I'm on my phone), but if you have a small test that fails that is ideally part of the commit and is tested to prevent future bugs.

@SteveDoyle2 SteveDoyle2 reopened this Nov 27, 2018
@SteveDoyle2 SteveDoyle2 merged commit f7dcd5b into SteveDoyle2:master Nov 27, 2018
@SteveDoyle2
Copy link
Owner

I don't see the message I just typed, so...

I ran your example and wow...my mistake. I completely misread that pull request.

I thought you changed the variable name from cMethods to cmethods and not the other way around.

@Msegade
Copy link
Contributor Author

Msegade commented Nov 28, 2018

Ok, glad is now solved.

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