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

DC Bias Points Bug Fix (Hackish) (READY) #286

Merged
merged 1 commit into from
Jun 26, 2015

Conversation

nvdl
Copy link
Contributor

@nvdl nvdl commented Jun 25, 2015

Loading data file is not skipped based on the previous loaded time.
This a current hack as 'Graph::loadDatFile()' does not support multi-node data loading.

This a current hack as 'Graph::loadDatFile()' does not support multi-node data loading.
@nvdl nvdl changed the title DC Bias Points Bug Fix (Hackish) DC Bias Points Bug Fix (Hackish) (READY) Jun 25, 2015
@nvdl
Copy link
Contributor Author

nvdl commented Jun 25, 2015

Let's check it and pull it so that it is not broken anymore.

@felix-salfelder
Copy link
Member

thanks

this is working for me.

for(node_it = NodeList.begin(); node_it != NodeList.end(); node_it++) {
qDebug() << "SweepDialog::slotNewValue:(*node_it)->Name:" << (*node_it)->Name;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This line should be after line 150.

@guitorri
Copy link
Member

Testing...

@guitorri
Copy link
Member

Works for me as well! Thank you!

guitorri added a commit that referenced this pull request Jun 26, 2015
DC Bias Points Bug Fix (Hackish) (READY)
@guitorri guitorri merged commit 14603e4 into Qucs:master Jun 26, 2015
@in3otd
Copy link
Contributor

in3otd commented Jun 26, 2015

it seems now we understand why the DC Bias Points stuff was not working - read data were still considered valid when asking for the data of another variable, IIUC.
Do we understand why it was working in some cases (small circuits, etc.)? I don't; if the above explanation is correct, is should have never worked...

@nvdl
Copy link
Contributor Author

nvdl commented Jun 26, 2015

@in3otd
I have been wondering about that as well but never tested that; no reference circuit.
Do you have a circuit where it works without this PR.
I can check that again.
Were there more than one nodes involved?

@in3otd
Copy link
Contributor

in3otd commented Jun 26, 2015

the schematic in #272 worked, if I removed one resistor. This was before felix's PR, did not check after.

@in3otd
Copy link
Contributor

in3otd commented Jun 26, 2015

...besides, the commit message is completely lacking some context; "Those who fail to document context in commit messages condemn all the developers to reread the code"...

@guitorri
Copy link
Member

Sorry, that was my mistake! I should have asked for a commit message...

@nvdl
Copy link
Contributor Author

nvdl commented Jun 26, 2015

Please write down the commit message here so that I can follow the logic and format (in the future).

@in3otd
Copy link
Contributor

in3otd commented Jun 26, 2015

I do not pretend to have captured exactly what was done, but I had in mind a commit message like this one:

Invalidate old data when looping to update DC Bias values.

When using 'Calculate DC bias' the loops used to compute the voltages and
currents to be shown in the schematic did not invalidate the previously used 
data. Due to the current implementation of `Graph::loadDatFile()` the data 
needed for the new node(s) were then not actually loaded but the previously loaded 
node data were considered valid instead. This caused Qucs to segfault as the
data pointer used was set to null by <etc., ...>
This is intended to be just a quick fix while the Graph and Diagram classes
are refactored and data loading separated from these classes.

I think the guidelines here and here sum up the main points nicely. And we know the author of the latter document is always right 😁 .

@nvdl
Copy link
Contributor Author

nvdl commented Jun 26, 2015

Good guidelines. I think, we should make it a requirement for a PR to fulfill.

@guitorri
Can you un-pull it so that I can follow the newly agreed upon way of creating a PR?
I agree, the comment is very important to be detailed; even for me in the future.

@guitorri
Copy link
Member

It is now merged. We cannot rewrite history.

@in3otd
Copy link
Contributor

in3otd commented Jun 26, 2015

will add the commit guidelines to our Contributing to Qucs wiki page.

@nvdl
Copy link
Contributor Author

nvdl commented Jun 26, 2015

One another thing regarding "clang-format", when will that be used?
Sometimes, I have to untangle the code but that obscures the review process in PR.
Thus, the effort put into untangling is lost to keep PR with minimal changes.

So I suggest, to make 2 commits; first commit being the changes and the second commit being the "clang-formatted" version of that file.

Either we format the code slowly in PRs or give it a full shot soon.

More ideas?

@in3otd
Copy link
Contributor

in3otd commented Jun 26, 2015

yes, some part of the code are badly formatted, but I too avoid to touch them if they are not part of the main PR subject.
The general idea was to format all the code in one shot shortly after 0.0.19 is released, which I guess should be Real Soon. We still need to decide the Coding Style to use.

@nvdl
Copy link
Contributor Author

nvdl commented Jun 26, 2015

We still need to decide the Coding Style to use.

Yes, this was on my mind as well but it was going to open a can of worms.

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.

4 participants