PieterTack / polycap

Polycapillary X-ray raytracing
GNU General Public License v3.0
3 stars 3 forks source link

Documentation: initial commit #37

Closed tschoonj closed 5 years ago

tschoonj commented 6 years ago

Heb meeste gekopieerd vanuit easyRNG zonder veel aan te passen. Heb al klein beetje gewerkt aan polycap-error.h

Om dit lokaal te testen moet je zien dat doxygen wordt opgepikt door configure (doe eerst autoreconf) Daarna zal je zien dat make op het einde de documentatie zal genereren in doc/ Doe dan firefox doc/html/index.html om te inspecteren...

PieterTack commented 6 years ago

Zo, denk dat alle public header functies nu hun documentation hebben. Nu nog de algemene documentatie :) Beetje vreemd ook dat in vorige commit er plots een check fail was. Iets van gcc dat hij moest downloaden en dan time out was. Vast niet onze schuld.

Hoe was/is EXRS? Al interesse gehad in onze poster? En in je eigen posters? Toevallig de persoon van XOS al gespot? Zou leuk zijn om de correcte profielen te krijgen voor de polycapillairen die wij hebben ;)

tschoonj commented 6 years ago

Hey Pieter,

Die build failure werd veroorzaakt door een probleem bij het downloaden van software of travis-ci. Dat gebeurt regelmatig: indien zo is het best dat je kan doen gewoon de build herstarten.

Succes van de polycap poster is moeilijk in te schatten aangezien ik niet ben weggeraakt van de xraylib en XMI-MSIM posters... volgende keer zal ik vragen al mijn posters naast mekaar te zetten of zelfs in aparte postersessies. Ben nog niet aangeklampt door XOS mensen...

tschoonj commented 6 years ago

Hey Pieter,

Heb wat wijzigingen aangebracht in extra_pages.dox en een paar van de headers.

Er zijn nog wel een paar functies die ofwel niet gedocumenteerd zijn, ofwel onvolledig. Ik krijg de volgende warnings:

/home/awf63395/github/polycap/include/polycap-error.h:97: warning: Member polycap_error_new_literal(enum polycap_error_code code, const char *message) (function) of file polycap-error.h is not documented.
/home/awf63395/github/polycap/include/polycap-error.h:99: warning: Member polycap_error_new_valist(enum polycap_error_code code, const char *format, va_list args) GNUC_PRINTF(2 (function) of file polycap-error.h is not documented.
/home/awf63395/github/polycap/include/polycap-error.h:101: warning: Member polycap_error_free(polycap_error *error) (function) of file polycap-error.h is not documented.
/home/awf63395/github/polycap/include/polycap-error.h:103: warning: Member polycap_error_copy(const polycap_error *error) (function) of file polycap-error.h is not documented.
/home/awf63395/github/polycap/include/polycap-error.h:105: warning: Member polycap_error_matches(const polycap_error *error, enum polycap_error_code code) (function) of file polycap-error.h is not documented.
/home/awf63395/github/polycap/include/polycap-error.h:107: warning: Member polycap_set_error(polycap_error **err, enum polycap_error_code code, const char *format,...) GNUC_PRINTF(3 (function) of file polycap-error.h is not documented.
/home/awf63395/github/polycap/include/polycap-error.h:109: warning: Member polycap_set_error_literal(polycap_error **err, enum polycap_error_code code, const char *message) (function) of file polycap-error.h is not documented.
/home/awf63395/github/polycap/include/polycap-error.h:111: warning: Member polycap_propagate_error(polycap_error **dest, polycap_error *src) (function) of file polycap-error.h is not documented.
/home/awf63395/github/polycap/include/polycap-error.h:113: warning: Member polycap_clear_error(polycap_error **err) (function) of file polycap-error.h is not documented.
Generating docs for file /home/awf63395/github/polycap/include/polycap-photon.h...
/home/awf63395/github/polycap/include/polycap-photon.h:76: warning: The following parameters of polycap_photon_launch(polycap_photon *photon, size_t n_energies, double *energies, double **weights, polycap_error **error) are not documented:
  parameter 'error'
Generating docs for file /home/awf63395/github/polycap/include/polycap-profile.h...
/home/awf63395/github/polycap/include/polycap-profile.h:90: warning: The following parameters of polycap_profile_new_from_file(const char *single_cap_profile_file, const char *central_axis_file, const char *external_shape_file, polycap_error **error) are not documented:
  parameter 'error'
Generating docs for file /home/awf63395/github/polycap/include/polycap-rng.h...
/home/awf63395/github/polycap/include/polycap-rng.h:43: warning: Member polycap_rng_new(void) (function) of file polycap-rng.h is not documented.
/home/awf63395/github/polycap/include/polycap-rng.h:50: warning: Member polycap_rng_new_with_seed(unsigned long int seed) (function) of file polycap-rng.h is not documented.
/home/awf63395/github/polycap/include/polycap-rng.h:56: warning: Member polycap_rng_free(polycap_rng *rng) (function) of file polycap-rng.h is not documented.
Generating docs for file /home/awf63395/github/polycap/include/polycap-source.h...
/home/awf63395/github/polycap/include/polycap-source.h:104: warning: return type of member polycap_source_get_transmission_efficiencies is not documented

Mag ik voorstellen om functies die gebruik maken van polycap_error te gaan documenteren zoals ik in deze commit gedaan heb voor polycap_source_get_photon:

/** Create a new random polycap_photon based on polycap_source
 *
 * In the event of an error, \c NULL is returned and \c error is set appropriately.
 * \param source a polycap_source
 * \param rng a polycap_rng
 * \param error a pointer to a \c NULL polycap_error, or \c NULL
 * \returns a new polycap_photon, or \c NULL if an error occurred
 */
polycap_photon* polycap_source_get_photon(polycap_source *source, polycap_rng *rng, polycap_error **error);

Sorry dat ik dit zelf niet doe, maar heb het momenteel bijzonder druk, en dit zal nog wel een week of twee duren...

PieterTack commented 6 years ago

no biggy, uiteraard heb je je eigen werk ook te doen :) Zal zoveel mogelijk aanpassen. De info over de polycap-error had ik bewust over geslaan, omdat ik weinig besef heb van die functies :) Zal er eens een metaforisch blik op werpen.

Moet ik echt * \param error a pointer to a \c NULL polycap_error, or \c NULL gebruiken? Is het niet beter om: * \param error a pointer to a polycap_error, or \c NULL te gebruiken? Kvind die NULL polycap_error wat vreemd... Of is dat net om aan te geven dat de polycap_error wel NULL dient te zijn? In dat geval houdt het soort van steek :)

tschoonj commented 6 years ago

Hmm nee, anders is het onjuist.

De gebruiker moet echt de polycap_error * eerst op NULL zetten, anders komt er miserie van op lijn 127-128 (en meer dan waarschijnlijk een segmentatie fout):

https://github.com/PieterTack/polycap/blob/18620af470884ef9119a42e7842d65ca01a44ea4/src/polycap-error.c#L112-L130

PieterTack commented 5 years ago

@tschoonj Misschien een quick help: als ik nu probeer te compilen als volgt: autoreconf ./configure CFLAGS="-g -O0 -Wall" LIBS="-L/usr/lib64 -lhdf5"

Krijg ik de error dat hij gsl package niet vindt. Ook als ik zonder CFLAGS of LIBS probeer krijg ik dezelfde foutmelding. Ik meen me vaag iets te herinneren dat we vroeger ook last hadden van die gsl package, maar kan me niet herinneren hoe we dat toen fixten. Enige dat ik kan vinden is dat we toen de autoconfigure ernaar lieten zoeken als verplichte dependency.

Kvermoed dat er ergens iets ontbreekt in mijn PATH ofzo, maar weet niet direct wat dan. Of is de gsl package verdwenen van de server? Is er een meer geautomatiseerde manier om deze packages te zoeken op het gehele systeem ofzo?

tschoonj commented 5 years ago

Doe je dit op xmi4? Op die machine zijn in elk geval GSL en zijn devel package geïnstalleerd.

Wat krijg je voor which pkg-config en echo $PKG_CONFIG_PATH?

Ik zie trouwens dat de Travis-CI macOS builds en de AppVeyor builds niet blij zijn... zal eens fixen later vandaag.

PieterTack commented 5 years ago

Idd. Appveyor geeft overigens zelfde probleem als mij met gsl

which pkg-config: /usr/local/anaconda-4.1.1/bin/pkg-config echo $PKG_CONFIG_PATH: /opt/devel/lib/pkgconfig:/usr/local/lib/pkconfig

tschoonj commented 5 years ago

AppVeyor is eigenlijk een ander probleem: de initiële update faalt door packages die niet meer bestaan waardoor alles faalt en GSL niet wordt geïnstalleerd. Heb vorige week hetzelfde gezien met de XMI-MSIM AppVeyor CI builds.

Verander uw PATH:

export PATH="/usr/bin:$PATH"

en probeer opnieuw...

PieterTack commented 5 years ago

That helps!

Nu geeft hij enkel nog een probleem over configure: WARNING: Incomplete python development package en erboven iets over dat linken van test program to Python niet lukt. Maybe the main Python library has been installed in some non-standard library path.

Bij make distcheck in de build folder geeft hij dan een issue omtrend doxygen, en dus doet hij ook de tests niet.

PieterTack commented 5 years ago

I made a boo-boo :( Bij mij wou compilatie dus nog steeds niet lukken, dus ik dacht om eens mijn build folder te cleanen. Had er dus make clean gedaan, maar als ik daarna make gebruikte kloed het programma dat ik in mijn source folder een make distclean moest uitvoeren. Waar ik deze ook uitvoerde, de foutmelding bleef dezelfde.

Uiteindelijk probeerde ik dus alles in build te deleten (op een hoop write-protected files na). Ik dacht immers dat met die ./configure deze build map terug werd aangemaakt, met alle relevante zaken in om daar het programma te kunnen builden. Blijkbaar niet dus ;) Hoe krijg ik deze build map terug in orde?

Ik vermoed overigens dat mijn issues er zijn door die Python development environment link dat hij niet kan vinden. Ik herinner me dat hij vroeger ook een waarschuwing over het niet vinden van een of andere versie van python, maar deze foutmelding zag ik nooit eerder.

tschoonj commented 5 years ago

Wat in build zit mag altijd verwijderd worden. Gebruik rm -f als er ambetant gedaan wordt over bepaalde bestanden.

Daarna gewoon terug ../configure laten lopen in build...

Negeer de Python warnings, dat kunnen we later eens bezien en is niet van belang voor de documentatie in elk geval.

By the way, vooraleer ge nu nieuwe commits zit te pushen naar GitHub, trek eerst de mijne in uw branch:

git fetch
git pull --rebase
PieterTack commented 5 years ago

Oke, fixed my own boo-boo. make clean en make distclean in mijn main folder hebben de boel geholpen :) Mijn issue bij compilatie is dus bij de doxygen functionaliteit. Bij make check lukken de tests, en daarna (als hij in /home/ptack/poly_raytrace/build/docs werkt) geeft hij een hoop warnings en uiteindelijk exit hij met Error1

tschoonj commented 5 years ago

Aha, ge had blijkbaar eens zitten builden in uw source... dat is dus geen goed idee! Altijd werken uit een build folder...

Ik snap niet waarom hij tijdens make check in docs ambetant doet... daar is niets te checken... Zl eens proberen op uw xmi4 van hieruit.

tschoonj commented 5 years ago

Ok de volgende setup werkt:

  1. Open een nieuwe terminal (belangrijk!)
  2. Voeg de gcc7 compilers toe aan uw PATH: export PATH=/usr/local/gcc/7.3.0/bin:$PATH
  3. Definieer PKG_CONFIG voor gebruik door configure: export PKG_CONFIG=/usr/bin/pkg-config
  4. Ga naar build
  5. autoreconf -fi ..
  6. ../configure && make clean && make && make check

Zou moeten werken...

PieterTack commented 5 years ago

Alright, dat werkt allemaal :) Thanks!

Mijn schema is nu dus om paper op te stellen rond dit programma. Wat ik er tot dusver zou in bespreken:

Heb jij nog ideeën van wat er zeker in moet, of leuk zou zijn om erin te zetten?

tschoonj commented 5 years ago

Nah, ziet er allemaal goed uit voor mij 😄

Btw, vooraleer er kan gereleased worden, moet er nog wat documentatie bijgeschreven worden in de headers... zin om daar eens naar te kijken bij gelegenheid?

PieterTack commented 5 years ago

Ja hoor, welke info is er missing? In de introductie ofzo ga ik toch beschrijven hoe het programma werkt (wat het berekent), dus kan dan even goed de headers info wat aanspekken :)

tschoonj commented 5 years ago

Wel als je de build doet waarbij de documentatie gegenereerd wordt, zal je een hoop warnings zien voor zaken die niet gedocumenteerd zijn.

tschoonj commented 5 years ago

Dit is wat ik krijg momenteel:

Generating docs for file /home/awf63395/github/polycap/include/polycap-description.h...
/home/awf63395/github/polycap/include/polycap-description.h:43: warning: argument 'sig_wave' of command @param is not found in the argument list of polycap_description_new(polycap_profile *profile, double sig_rough, int64_t n_cap, unsigned int nelem, int iz[], double wi[], double density, polycap_error **error)
/home/awf63395/github/polycap/include/polycap-description.h:43: warning: argument 'corr_length' of command @param is not found in the argument list of polycap_description_new(polycap_profile *profile, double sig_rough, int64_t n_cap, unsigned int nelem, int iz[], double wi[], double density, polycap_error **error)
Generating docs for file /home/awf63395/github/polycap/include/polycap-error.h...
Generating docs for file /home/awf63395/github/polycap/include/polycap-photon.h...
/home/awf63395/github/polycap/include/polycap-photon.h:85: warning: Member polycap_leaks (typedef) of file polycap-photon.h is not documented.
/home/awf63395/github/polycap/include/polycap-photon.h:81: warning: The following parameters of polycap_photon_launch(polycap_photon *photon, size_t n_energies, double *energies, double **weights, bool leak_calc, polycap_error **error) are not documented:
  parameter 'leak_calc'
Generating docs for file /home/awf63395/github/polycap/include/polycap-profile.h...
Generating docs for file /home/awf63395/github/polycap/include/polycap-rng.h...
Generating docs for file /home/awf63395/github/polycap/include/polycap-source.h...
/home/awf63395/github/polycap/include/polycap-source.h:56: warning: The following parameters of polycap_source_new(polycap_description *description, double d_source, double src_x, double src_y, double src_sigx, double src_sigy, double src_shiftx, double src_shifty, double hor_pol, size_t n_energies, double *energies, polycap_error **error) are not documented:
  parameter 'hor_pol'
  parameter 'n_energies'
  parameter 'energies'
/home/awf63395/github/polycap/include/polycap-source.h:95: warning: argument 'n_energies' of command @param is not found in the argument list of polycap_source_get_transmission_efficiencies(polycap_source *source, int max_threads, int n_photons, bool leak_calc, polycap_progress_monitor *progress_monitor, polycap_error **error)
/home/awf63395/github/polycap/include/polycap-source.h:95: warning: argument 'energies' of command @param is not found in the argument list of polycap_source_get_transmission_efficiencies(polycap_source *source, int max_threads, int n_photons, bool leak_calc, polycap_progress_monitor *progress_monitor, polycap_error **error)
/home/awf63395/github/polycap/include/polycap-source.h:108: warning: The following parameters of polycap_source_get_transmission_efficiencies(polycap_source *source, int max_threads, int n_photons, bool leak_calc, polycap_progress_monitor *progress_monitor, polycap_error **error) are not documented:
  parameter 'leak_calc'

Als je hiermee klaar bent zal ik eens kijken naar de docs voor de Python bindings.

tschoonj commented 5 years ago

Hey Pieter,

Ik stel voor om deze pull request te mergen aangezien we toch wel wat aan het afwijken zijn van het oorspronkelijke doel ervan (documentatie). Of misschien nog een commit met wat laatste documentatie verbeteringen?

PieterTack commented 5 years ago

Hoi, Kweet dat ik beetje off-topic ging, maar was bezig met de relevante figuren voor de paper te maken en ik stuitte op enkele issues die niet echt klopten. Heb ze dus snel gefixed. Alles komt nu meer uit volgens de verwachtingen (al had ik toch een nog sterkere stijging van leak events in functie van energie verwacht, maar soit)

Ik was idd van plan om de komende dagen nog de documentatie wat aan te vullen die momenteel ontbreekt. Daarna kunnen we voor mijn part de PR mergen.

tschoonj commented 5 years ago

Ok, cool.

Als je niet helemaal zeker bent over de correctheid van de resultaten, vraag misschien eens Laszlo om advies.

tschoonj commented 5 years ago

Ipv de functie uit te commentariëren had je ze misschien best verplaatst naar te test zelf, als dit nuttig zou geweest zijn...

PieterTack commented 5 years ago

Aja, was nog een idee geweest :) Wou ze gewoon niet volledig uit de code wissen omdat het op zich niet fout is, en omdat het de functie is die origineel in Laszlo's code gebruikt werd (dus kan gebruikt worden voor varity checks mocht later relevant zijn of wat dan ook).

tschoonj commented 5 years ago

Wat in een git repo zit kan nooit volledig gewist worden: je kan altijd eerdere commits reverten.

Merci trouwens voor de paper: ik zal het eerstdaags eens grondig lezen en becommentariëren.

PieterTack commented 5 years ago

Sorry dat ik het in deze pull request commit. Kweet dat hier niet thuis hoort. Maar kvond niet hoe ik, eens ik al changes heb gemaakt aan een lokale branch, deze dan moet comitten en pushen naar een nieuwe pull request (of exporten naar nieuwe branch ofzo). Ksloeg er nu overigens niet eens meer in om een nieuwe pull request te maken, google hielp me ook niet veel verder... (kon enkel branches vergelijken, maar nergens had ik de optie om iets nieuws te maken...)

tschoonj commented 5 years ago

Geen probleem... best dat we gewoon deze PR zo snel mogelijk mergen.

tschoonj commented 5 years ago

Best dat je eens kijkt naar die failure op AppVeyor:

polycap-source.c: In function 'polycap_source_get_transmission_efficiencies':
1051polycap-source.c:764:4: error: this 'if' clause does not guard... [-Werror=misleading-indentation]
1052  764 |    if(iesc == 2) //photon never entered PC (hit capillary wall instead of opening)
1053      |    ^~
1054polycap-source.c:771:5: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
1055  771 |     photon->exit_coords.x = photon->src_start_coords.x + source->d_source*photon->start_direction.x/photon->start_direction.z;
1056      |     ^~~~~~
1057polycap-source.c:779:5: error: implicit declaration of function 'polycap_capil_reflect'; did you mean 'polycap_capil_trace'? [-Werror=implicit-function-declaration]
1058  779 |     polycap_capil_reflect(photon, acos(polycap_scalar(central_axis,photon->exit_direction)), central_axis, leak_calc, NULL);
1059      |     ^~~~~~~~~~~~~~~~~~~~~
1060      |     polycap_capil_trace
PieterTack commented 5 years ago

The memory error appears within the calls to polycap_leak_free(). Either I'm calling these functions twice on the same struct, or they are not properly freeing, or ... In any case, if I comment the calls out (lines 844,845 and 863,864 in polycap_source.c), the calculation appears to proceed without memory issues (or at least without core dumps or such)

tschoonj commented 5 years ago

valgrind geprobeerd?

PieterTack commented 5 years ago

Ja, maar die geeft uiteindelijk enkel segmentation fault, geen error op die lijn ofzo Krijg wel: glibc detected /home/ptack/poly_raytrace/build/src/.libs/polycap: double free or corruption (out): en dan ongeveer hetzelfde met telkens malloc(): memory corruption, munmap_chunk(): invalid pointer, corrupted double-linked list, tot uiteindelijk een segmentation fault

PieterTack commented 5 years ago

en Valgrind geeft nu overigens ook precies een hoop Heap en Leak summaries, met 'still reachable' bytes, maar allemaal nog voor het programma eigenlijk de berekening start. Dat deed hij vroeger volgens mij niet, maar heb aan dat deel niks verandert (tenzij het in de included headers problematisch wordt ofzo)

tschoonj commented 5 years ago

zal er eens snel naar zien

PieterTack commented 5 years ago

Heb het mss gevonden, door nadat ik de polycap_leak_free functie roep ook telkens recap_temp of leaks_temp == NULL te zetten. Meen me iets te herinneren met realloc die enkel nieuw geheugen malloc als de pointer NULL is?

tschoonj commented 5 years ago

Mja, dat is potentieel gevaarlijk: als je na polycap_leak_free die pointer hergebruikt moet die eerst op NULL gezet worden anders gaat realloc een segfault geven vroeg of laat.

Ben nog altijd source aan het draaien met valgrind... duurt een tijdje

PieterTack commented 5 years ago

Ah, maar source op zich gaat (normaal) geen issues geven. De issue zit in de leak calculation, maar de huidige tests werken altijd zonder de leak calculation. Daarmee dat ik een Issue toevoegde om eens tests mét leak calculation toe te voegen ;) Kmoet wel zeggen dat ik nu alle mogelijke leaks heb toegevoegd, dus de simulatie duurt nu echt wel ferm lang met leaks :P Khad wel de indruk van preliminaire resultaten dat de verbeterde leak berekeningen wel eens die achtergrond zouden kunnen veranderen (wat je nu zo vond afwijken tussen praktijk en theorie in de paper)

tschoonj commented 5 years ago

valgrind ./source is nu afgelopen maar heeft geen problemen gevonden...

tschoonj commented 5 years ago

Als zou blijken dat die leaks zo een groot verschil zouden geven bij de berekening... misschien ze dan verplicht maken ipv optioneel?

PieterTack commented 5 years ago

Als het inderdaad zo'n groot verschil maakt is het zeker een goed idee om verplicht te maken. Maar eerst kijken of het effect inderdaad zo groot is (voornamelijk voor wat ik de internal leaks noem in de paper, de external worden toch door housing geabsorbeerd normaal), en of ik na al deze veranderingen nog steeds een goede overeenkomst heb met de experimentele waarden ;)

PieterTack commented 5 years ago

can be merged

tschoonj commented 5 years ago

Even Wachten op de CI builds, en als die groen zijn, merge ik in.

PieterTack commented 5 years ago

ah, ze waren groen :) Maar ben dan dom geweest en had PR geclosed en weer geopend, waardoor hij de checks opnieuw doet :)

tschoonj commented 5 years ago

Aangezien ik de PR heb geopend moet gij de review geven en mergen 😄

tschoonj commented 5 years ago

👍