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

Lay aside phasor and waveac diagrams #760

Merged
merged 2 commits into from
Feb 5, 2018
Merged

Conversation

andresmmera
Copy link
Contributor

The aim of this commit is to lay aside the waveac and phasor diagrams since their current implementation makes much more difficult to merge older PR (specifically #370) and entangles the Qucs code.

In this sense, "phasordiagram.h/.cpp" and "waveac.h/.cpp" were removed from "CMakeLists.txt" and "Makefile.am". The most relevant code spread over "diagram.cpp/h", "marker.cpp/h", etc. was
commented whereas those portions of code that intrude existing functions were removed.
IMHO, an alternative non-intrusive way will be needed to implement these features in the future.

The aim of this commit is to lay aside the waveac and phasordiagram
code. This code is difficultinh the merge of old PRs and entagles
(even more) the Qucs code. In this sense, phasordiagram.h/.cpp and
waveac.h/.cpp were removed from "CMakeLists.txt" and "Makefile.am". The
most relevant code spread over diagram.cpp/h, marker.cpp/h, etc. was
commented whereas those portions of code that intrude
existing functions were removed since it'll be needed to figure out an
alternative non-intrusive way to implement that feature.
@andresmmera
Copy link
Contributor Author

Travis says:

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

The build has been terminated

I restarted the build but I got the same result. Any guess?

@in3otd
Copy link
Contributor

in3otd commented Dec 16, 2017

I see both builds get stuck at the same point, after the qucsator test, I'm checking locally to see how it behaves.

I fear that once the code is out it will be quite difficult to find the time to refactor it merge it back in; do we have at least some examples to see how these diagrams should work?

@andresmmera
Copy link
Contributor Author

andresmmera commented Dec 16, 2017

Yes, you can check how the diagrams look like in this document (taken from the #682 discussion thread) which contains two examples https://github.com/Qucs/qucs/files/998064/Diagrams.examples.pdf

I fear that once the code is out it will be quite difficult to find the time to refactor it merge it back in; do we have at least some examples to see how these diagrams should work?

I fully agree. However, I believe this is the lesser evil. The code related to these features spreads over several files making a lot of conditionals that ends up entangling the (already entangled) diagram code. For example,

https://github.com/qucs/qucs/blob/develop/qucs/qucs/diagrams/diagram.cpp#L62
https://github.com/qucs/qucs/blob/develop/qucs/qucs/diagrams/diagram.cpp#L198
https://github.com/qucs/qucs/blob/develop/qucs/qucs/diagrams/diagram.cpp#L698

https://github.com/qucs/qucs/blob/develop/qucs/qucs/diagrams/diagramdialog.cpp#L294
https://github.com/qucs/qucs/blob/develop/qucs/qucs/diagrams/diagramdialog.cpp#L748
https://github.com/qucs/qucs/blob/develop/qucs/qucs/diagrams/diagramdialog.cpp#L769
https://github.com/qucs/qucs/blob/develop/qucs/qucs/diagrams/diagramdialog.cpp#L821

etc...

IMHO, I shouldn't have merged #682, but now it's done, the least thing I can do is to restore somehow the previous code...

@in3otd
Copy link
Contributor

in3otd commented Dec 16, 2017

I'm not familiar with the test stuff but I see something strange in the build logs around

Running [qucs]:  qucs -n -i testsuite/AC_groupdelay_ac_prj/groupdelay_ac.sch -o testsuite/AC_groupdelay_ac_prj/test_groupdelay_ac.txt
Comparing : diff testsuite/AC_groupdelay_ac_prj/netlist.txt testsuite/AC_groupdelay_ac_prj/test_groupdelay_ac.txt
�[92mDiff netlist    : PASS�[0m
�[94mConverting schematic to netlist.�[0m
Running [qucs]:  qucs -n -i testsuite/AC_phasordiag_prj/phasordiag.sch -o testsuite/AC_phasordiag_prj/test_phasordiag.txt

Project :  /home/travis/build/Qucs/qucs/qucs-0.0.19/_build/qucs-test/testsuite/DC_TR_active_mixer_prj

there is an AC_phasordiag_prj test, which I think should just fail now

@andresmmera
Copy link
Contributor Author

Thanks!! I've just removed the two tests related to PR682

@in3otd
Copy link
Contributor

in3otd commented Dec 16, 2017

running distcheck locally I see why the build/test got stuck: once it gets to the removed diagram test it opens an error box message saying "Format Error: Unknown diagram!" with an Ok button, which of course cannot be clicked by the script...
After clicking the Ok button, the script fails as expected.

This is of course somewhat related to #756 - X should not be needed for running the tests.

BTW, Travis still fails, but with a slightly different error now

@andresmmera andresmmera force-pushed the RemovePR682 branch 2 times, most recently from cfdcb34 to cd73a37 Compare December 16, 2017 14:44
@andresmmera
Copy link
Contributor Author

Surely I'm failing to understand the logic behind the git submodules... but I followed the procedure detailed here and it seems that the changes I'm doing locally are not commited/pushed correctly.

Basically, I did the following:
cd qucs-test/
git checkout master
git pull
git rm -r testsuite/AC_temporaldiag_prj/
git rm -r testsuite/AC_phasordiag_prj/
git rm -r testsuite/DC_AC_ohmmeter_prj/
git rm -r testsuite/DC_AC_internal_resistance_prj/
cd ..
git add qucs-test/
git commit --amend

Am I missing something important? 😓

@in3otd
Copy link
Contributor

in3otd commented Dec 16, 2017

I think that the changes to qucs-test should be committed to the qucs-test repo first, then you can update the qucs submodule pointer to qucs-test.
The changes you did in qucs-test above are just staged in your local repo, the remote repo is unchanged; the commit just "updated" the qucs submodule pointer to point to the current commit in qucs-test, which was actually not changed (nothing was committed to qucs-test)

@in3otd
Copy link
Contributor

in3otd commented Dec 16, 2017

and maybe, since this should be a temporary thing, you could leave the tests in place and just add them to the exclusions lists skip.txt and skip_OSX.txt

@andresmmera andresmmera force-pushed the RemovePR682 branch 2 times, most recently from c3ac4d9 to 6746420 Compare December 17, 2017 08:36
@guitorri guitorri added this to the 0.0.20 milestone Feb 5, 2018
@guitorri guitorri removed the proposal label Feb 5, 2018
guitorri added a commit to Qucs/qucs-test that referenced this pull request Feb 5, 2018
@guitorri
Copy link
Member

guitorri commented Feb 5, 2018

I fixed it for you. Hope it passes now.

With submodules I do the following:

  1. create branch and commit locally to the qucs-test
  2. push qucs-test branch to remote (so other can see your commits)
  3. from qucs create branch, git add qucs-test, commit (record new qucs-test hash) and push to remote (to start Travis).

@andresmmera
Copy link
Contributor Author

@guitorri Many thanks!! I found particularly difficult to update the submodules. Thank you for those guidelines!! I'm sure they'll help me in the future :-)

@guitorri
Copy link
Member

guitorri commented Feb 5, 2018

I will merge as is. If the Graph refactor is still hard to merge we might need to clean it up further.

@guitorri guitorri merged commit 1d7a41b into Qucs:develop Feb 5, 2018
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