search

JMSLab / Template

Template for research repository using scons.
9 stars 1 forks source link

Handout functionality for LyX builder #49

Closed veli-m-andirin closed 2 years ago

veli-m-andirin commented 2 years ago

In this issue we'll consider adding a handout functionality to LyX builder that can be invoked to eliminate pauses and produce an output file that includes the notes.

It will be useful to check the modules that the following script refers to:

###########################################
# GET LIBRARY
###########################################
import os
from gslab_make.make_log import *
from gslab_make.run_program import * 
from gslab_make.dir_mod import * 

###########################################
# HANDOUT.PY STARTS
###########################################
# SET DEFAULT OPTIONS
set_option(makelog = './temp/make.log',
           temp_dir = './temp/')

clear_dirs('./temp/')
start_make_logging()

# COMPILE PDF FILES
run_lyx(program = 'protest_slides.lyx',
        handout = True,
        comments = True,
        pdfout = './temp/protest_slides.pdf')

end_make_logging()

input('\n Press <Enter> to exit.')
rcalvo12 commented 2 years ago

@veli-m-andirin @jmshapir

I have completed a first pass at the handout functionality for the lyx builder. I added a directory to /source/ and /output/ called /talk/ where I included a template for slides in a .lyx file as well as a relevant SConscript file.

To use:

If you clear out the /output/talk/ directory and then run scons from the root of the directory, you will see the handout and clean versions are produced. If you go to /source/talk/, remove '#output/talk/talk_slides_handout.pdf' from the SConscript, clear the /output/talk/directory again, and run scons again, you will see that just the clean version is produced with no errors.

Would love to hear your thoughts and feedback on this!

veli-m-andirin commented 2 years ago

Thanks @rcalvo12 for great work! I had a quick look at the code and did some testing. The only issues I spotted are:

If you specify the clean slides first, and then the handout, everything seems to work well. I think it would be great if we could fix the cases above, too. Please let me know at any moment where some input could be helpful.

jmshapir commented 2 years ago

@rcalvo12 nice work, thanks! I ran this on my end and it compiled successfully on the first try.

(And thanks @veli-m-andirin for the comments!)

@rcalvo12 a few thoughts:

From my standpoint once everything is working and we have some basic unit testing, we can go to a pull here and add @veli-m-andirin as reviewer.

rcalvo12 commented 2 years ago

Thanks @veli-m-andirin and @jmshapir for the comments. On the comments by @jmshapir :

  • I think the metropolis theme does not ship with LyX and needs to be installed "by hand." Maybe we should comment it out to avoid adding another dependency to this repo? Will do!

  • Do we want the handout to be stored by default in /output or just in /temp?

I don't have a strong preference, I was matching the Protest repo where we are storing it in output. Given that it is part of the template, I think there is a case for storing the output in a directory tracked by git.

Understood.

  • Using the filename (_handout) to control behavior is nice but I could also see advantages in just asking the user to pass an explicit option to the LyX builder to add a handout. Do you have thoughts on the tradeoffs between these alternative designs?

The approach I took felt more natural to implement given my prior (limited) experience with this stuff. I am trying my hand at the approach you suggest here and to we can see what the tradeoffs would be if/when I get it working.

From my standpoint once everything is working and we have some basic unit testing, we can go to a pull here and add @veli-m-andirin as reviewer.

Sounds good!

On the comments from @veli-m-andirin, I will approach debugging this as part of exploring the different mechanism for controlling behavior.

jmshapir commented 2 years ago

Thank @rcalvo12!

I don't have a strong preference, I was matching the Protest repo where we are storing it in output.

I think in Protests we have the handout in /temp?

Given that it is part of the template, I think there is a case for storing the output in a directory tracked by git.

I agree, I can see a case either way and likewise don't feel strongly. My current instinct is to store in /temp to reduce the potential for confusion about which PDF should be used for presentation.

rcalvo12 commented 2 years ago

Thanks @jmshapir!

I think in Protests we have the handout in /temp?

Apologies, yes, the talk folder only stores the clean version.

I agree, I can see a case either way and likewise don't feel strongly. My current instinct is to store in /temp to reduce the potential for confusion about which PDF should be used for presentation.

I will go ahead and rework so that it is stored in /temp/.

jmshapir commented 2 years ago

Adding @ew487 to hopefully wrap up work here, thanks!

ew487 commented 2 years ago

Hi @jmshapir @rcalvo12 , I have a version of this working on my computer now. This version is the same as before, except that the variable controlling handout functionality is a construction variable now (here called HANDOUT_BOOL) instead of a regular variable:

Please let me know what you think!

rcalvo12 commented 2 years ago

@ew487 This looks great! Can you push your code so we can see how it runs on our end?

ew487 commented 2 years ago

@rcalvo12 Sure! Could you make me an editor on this project so I can push the code?

rcalvo12 commented 2 years ago

@rcalvo12 Sure! Could you make me an editor on this project so I can push the code?

@jmshapir can you give @ew487 write access?

jmshapir commented 2 years ago

Done! Use it wisely @ew487!

ew487 commented 2 years ago

@jmshapir @rcalvo12 Got it, thanks! I just pushed the changes. To test it out, you can try changing the value of the construction variable HANDOUT_BOOL. True strings should make a handout pdf in the temp folder. False strings or no specification given (as in env.Lyx(target, source)) should not make the handout pdf.

ew487 commented 2 years ago

Update: I'm running into some errors when I'm trying to create a handout of the paper, i.e. when I have env.Lyx(target, source, HANDOUT_BOOL=1) in ./source/paper/Sconscript, so I will look into this.

jmshapir commented 2 years ago

@ew487 that sounds great. Once we find the source of the error we might consider adding a unit test that would have caught it had it been in place. That way we're building out our testing architecture alongside the user-side functionality itself.

rcalvo12 commented 2 years ago

@ew487 Looking through I think the functionality is pretty much what we were aiming for, other than the issue which you have spotted here which seems to be related to bringing in figures.

When it comes to the unit-testing mentioned here, it will likely be useful for you to look at the threads on unit testing in #56 .

ew487 commented 2 years ago

@rcalvo12 Got it! Thanks for this info!

ew487 commented 2 years ago

Hi @rcalvo12 , I've fixed the problem with bringing in figures. The revised code generates the handout pdf in the output folder and then moves it to temp at the end, instead of generating it in temp directly.

I also had a couple of questions I was hoping you could help with:

  1. Are we specially handling the case where the target is empty? I am wondering because I saw this if-else statement in this version of build_lyx.pdf: 

    if bool(target):
        target = misc.make_list_if_string(target)
        target_file = os.path.split(str(target[0]))[1]
        target_name = os.path.splitext(target_file)[0]
    else:
        target_name   = ''

  2. Right now my code is such that if I run Scons from the command line with the handout control variable turned off in source/talk/Sconscript, a pdf of the slides is created in output and no handout is created. If I then turn the handout control variable on and run Scons again without clearing the output folder, there is still no handout in temp since no rebuild was triggered. Is this the desired behavior? Or do we want to add explicit dependencies to trigger rebuild of the pdf+handout every time the relevant Sconscript file changes:

jmshapir commented 2 years ago

@ew487 on (2), my instinct is that if the user is planning to produce the "handout," the user should probably specify the handout file as a target. (A given file can be at once a target for scons and ignored by git, say because it is in a temp folder.)

In terms of workflow, once @rcalvo12 answers (1), and you have a version of the code and unit testing that you're happy with, I'd say you can go ahead and open a pull request with @rcalvo12 @jmshapir as reviewers. We can always continue some iteration and fine-tuning in the pull.

Thanks!

rcalvo12 commented 2 years ago
  1. Are we specially handling the case where the target is empty? I am wondering because I saw this if-else statement in this version of build_lyx.pdf: if bool(target):    target = misc.make_list_if_string(target)    target_file = os.path.split(str(target[0]))[1]    target_name = os.path.splitext(target_file)[0]else:    target_name = ''

@ew487 I don't believe we are explicitly handling this because if the target name is empty, the lyx builder will already throw an error because scons doesn't know what to build. If anything, these lines might be a bit redundant.

@ew487 on (2), my instinct is that if the user is planning to produce the "handout," the user should probably specify the handout file as a target. (A given file can be at once a target for scons and ignored by git, say because it is in a temp folder.)

@jmshapir, In previous iterations we specified the handout as a target, where the builder would recognize that the user wanted to make a handout if there was a target called *_handout. The last version with this functionality is in https://github.com/JMSLab/Template/commit/9cd74497a077625bc9a874fe6e0300ff94dd7ab7 and the function was implemented in https://github.com/JMSLab/Template/commit/3b3ef699ab5164cfe8b031dcb5bda8c3323ad7d2 . If my memory is correct, with this structure, specifying the new target meant that scons would track and build the handout even if the non-handout version was already built.

My impression was that the change you requested here in point four was to avoid having the handout explicitly named as a target and to just give a specific option to the builder that generates the handout if set to a TRUE. Now as part of that comment, you also noted that we would have to consider the tradeoffs of each type of implementation which was hard to do before having a working version of the second option. If your vision was that the user would have to specify the handout as a target file then, I think the older versions accomplish that and naturally work well with scons because it gives scons a new explicit target to track. The new version has a convenience factor because you just have to add a simple option to the function, but you do lose some control. You have to build to temp, you have to delete the non-handout version before you can build the handout version with scons. And if we were also going to have to specify a target on top of the boolean option, we have just added an element that the user would have to remember if they wanted to build a handout, which would actually make this version less convenient than the original iteration.

@jmshapir do you think reverting to the previous version would be preferable now that we have seen both iterations of this builder?

@ew487 fyi.

jmshapir commented 2 years ago

Thanks @rcalvo12! That's very helpful.

It seems like we have two questions to consider.

One is whether to ask the user to explicitly request a handout, or to have it be implied by the target structure/naming. My instinct is still towards having the user explicitly request a handout. It's a little less convenient for the user but adds a little more robustness in terms of expressing the user's intent.

The other is whether, in cases where the user requests a handout, we want the handout to be declared as a target to scons, in addition to the main / non-handout version of the PDF. I don't feel too strongly about this, but my sense after thinking about @ew487's question is that we may get more intuitive behavior from scons if we declare the target, because then, e.g., we don't have to "trick" scons into building it. It would also allow the user to pick any location they want for the handout, which would add flexibility if e.g. the user doesn't want the handout in /temp.

Does that sound reasonable @rcalvo12?

rcalvo12 commented 2 years ago

Thanks @jmshapir: Per offline conversation with @ew487 and phone call with @jmshapir this is the structure we are envisioning: image

In words, the user has to explicitly name their handout file and include a target with that same name in order to produce a handout. If the user doesn't specify the handout file name, it is not built. If the user specifies the handout name but there isn't a matching name in the list of targets, it is not built and an error is thrown.

ew487 commented 2 years ago

Thanks @jmshapir @rcalvo12 ! I will have a go at implementing this now.

ew487 commented 2 years ago

I want to flag that in current and previous versions of the code (see image) the ordering of elements in the target list does seem to matter, since the solution we discussed on the call earlier today was partially to address the possibility that the elements might be out of order. I can try to revise this code

so that order no longer matters; or alternatively, if we decided to keep that code as is, maybe it is now safe to assume that the second element is the path to the handout since we are already assuming the first element is the path to the clean pdf.

jmshapir commented 2 years ago

Thanks @ew487! My impression is that order matters in many of our builders so I think it is ok to allow that here, too.

How about we say that:

Then we don't need a handout "switch" at all. This structure will look a little more implicit from the user side but it seems flexible enough for the use cases I can envision.

@ew487 if you don't see any issues with this, I propose you implement, and if @rcalvo12 has concerns we can iterate in the pull!

Thanks again both, including for the call earlier today.

ew487 commented 2 years ago

Hi @jmshapir @rcalvo12, to summarize our offline conversation, we have decided on this behavior:

If something is not right here please let me know!

rcalvo12 commented 2 years ago

Thread continues in #62 .

ew487 commented 2 years ago

Summary

In this issue added a handout option to the Lyx builder. last commit 9408134 behavior summarized in https://github.com/JMSLab/Template/issues/49#issuecomment-1192929420